[PATCH v7 0/7] Fix PF/VF dependency issue

[PATCH v7 0/7] Fix PF/VF dependency issue

[sathyanarayanan.kuppuswamy]

From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

Current implementation of ATS, PASID, PRI does not handle VF dependencies
correctly. Following patches addresses this issue.

Changes since v6:
 * Removed locking interfaces used in v6.
 * Made necessary changes to PRI/PASID enable/disable calls to allow
   register changes only for PF.

Changes since v5:
 * Created new patches for PRI/PASID capability caching.
 * Removed individual locks (pri_lock, pasid_lock) and added common
   resource lock to synchronize PRI/PASID updates between PF/VF.
 * Addressed comments from Bjorn Helgaas.

Changes since v4:
 * Defined empty functions for pci_pri_init() and pci_pasid_init() for cases
   where CONFIG_PCI_PRI and CONFIG_PCI_PASID are not enabled.

Changes since v3:
 * Fixed critical path (lock context) in pci_restore_*_state functions.

Changes since v2:
 * Added locking mechanism to synchronize accessing PF registers in VF.
 * Removed spec compliance checks in patches.
 * Addressed comments from Bjorn Helgaas.

Changes since v1:
 * Added more details about the patches in commit log.
 * Removed bulk spec check patch.
 * Addressed comments from Bjorn Helgaas.

Kuppuswamy Sathyanarayanan (7):
  PCI/ATS: Fix pci_prg_resp_pasid_required() dependency issues
  PCI/ATS: Cache PRI capability check result
  PCI/ATS: Cache PASID capability check result
  PCI/ATS: Add PRI support for PCIe VF devices
  PCI/ATS: Add PASID support for PCIe VF devices
  PCI/ATS: Disable PF/VF ATS service independently
  PCI: Skip Enhanced Allocation (EA) initialization for VF device

 drivers/pci/ats.c       | 202 ++++++++++++++++++++++++++++------------
 drivers/pci/pci.c       |   7 ++
 include/linux/pci-ats.h |  12 ++-
 include/linux/pci.h     |   3 +-
 4 files changed, 159 insertions(+), 65 deletions(-)

-- 
2.21.0

[PATCH v7 1/7] PCI/ATS: Fix pci_prg_resp_pasid_required() dependency issues

[sathyanarayanan.kuppuswamy]

From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

Since pci_prg_resp_pasid_required() function has dependency on both
PASID and PRI, define it only if both CONFIG_PCI_PRI and
CONFIG_PCI_PASID config options are enabled.

Fixes: e5567f5f6762 ("PCI/ATS: Add pci_prg_resp_pasid_required()
interface.")
Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 drivers/pci/ats.c       | 10 ++++++----
 include/linux/pci-ats.h | 12 +++++++++---
 2 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
index e18499243f84..cdd936d10f68 100644
--- a/drivers/pci/ats.c
+++ b/drivers/pci/ats.c
@@ -395,6 +395,8 @@ 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.
@@ -402,10 +404,8 @@ 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)
 {
@@ -425,6 +425,8 @@ int pci_prg_resp_pasid_required(struct pci_dev *pdev)
 }
 EXPORT_SYMBOL_GPL(pci_prg_resp_pasid_required);
 
+#endif
+
 #define PASID_NUMBER_SHIFT	8
 #define PASID_NUMBER_MASK	(0x1f << PASID_NUMBER_SHIFT)
 /**
diff --git a/include/linux/pci-ats.h b/include/linux/pci-ats.h
index 1ebb88e7c184..1a0bdaee2f32 100644
--- a/include/linux/pci-ats.h
+++ b/include/linux/pci-ats.h
@@ -40,7 +40,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 */
 
@@ -67,11 +66,18 @@ static inline int pci_max_pasids(struct pci_dev *pdev)
 	return -EINVAL;
 }
 
+#endif /* CONFIG_PCI_PASID */
+
+#if defined(CONFIG_PCI_PRI) && defined(CONFIG_PCI_PASID)
+
+int pci_prg_resp_pasid_required(struct pci_dev *pdev);
+
+#else /* CONFIG_PCI_PASID && CONFIG_PCI_PRI */
+
 static inline int pci_prg_resp_pasid_required(struct pci_dev *pdev)
 {
 	return 0;
 }
-#endif /* CONFIG_PCI_PASID */
-
+#endif
 
 #endif /* LINUX_PCI_ATS_H*/
-- 
2.21.0

Re: [PATCH v7 1/7] PCI/ATS: Fix pci_prg_resp_pasid_required() dependency issues

[Bjorn Helgaas]

On Wed, Aug 28, 2019 at 03:14:01PM -0700, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> 
> Since pci_prg_resp_pasid_required() function has dependency on both
> PASID and PRI, define it only if both CONFIG_PCI_PRI and
> CONFIG_PCI_PASID config options are enabled.
> 
> Fixes: e5567f5f6762 ("PCI/ATS: Add pci_prg_resp_pasid_required()
> interface.")

[Don't split tags, including "Fixes:" across lines]

This definitely doesn't fix e5567f5f6762.  That commit added
pci_prg_resp_pasid_required(), but with no dependency on
CONFIG_PCI_PRI or CONFIG_PCI_PASID.

This patch is only required when a subsequent patch is applied.  It
should be squashed into the commit that requires it so it's obvious
why it's needed.

I've been poking at this series, and I'll post a v8 soon with this and
other fixes.

> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> ---
>  drivers/pci/ats.c       | 10 ++++++----
>  include/linux/pci-ats.h | 12 +++++++++---
>  2 files changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
> index e18499243f84..cdd936d10f68 100644
> --- a/drivers/pci/ats.c
> +++ b/drivers/pci/ats.c
> @@ -395,6 +395,8 @@ 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.
> @@ -402,10 +404,8 @@ 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)
>  {
> @@ -425,6 +425,8 @@ int pci_prg_resp_pasid_required(struct pci_dev *pdev)
>  }
>  EXPORT_SYMBOL_GPL(pci_prg_resp_pasid_required);
>  
> +#endif
> +
>  #define PASID_NUMBER_SHIFT	8
>  #define PASID_NUMBER_MASK	(0x1f << PASID_NUMBER_SHIFT)
>  /**
> diff --git a/include/linux/pci-ats.h b/include/linux/pci-ats.h
> index 1ebb88e7c184..1a0bdaee2f32 100644
> --- a/include/linux/pci-ats.h
> +++ b/include/linux/pci-ats.h
> @@ -40,7 +40,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 */
>  
> @@ -67,11 +66,18 @@ static inline int pci_max_pasids(struct pci_dev *pdev)
>  	return -EINVAL;
>  }
>  
> +#endif /* CONFIG_PCI_PASID */
> +
> +#if defined(CONFIG_PCI_PRI) && defined(CONFIG_PCI_PASID)
> +
> +int pci_prg_resp_pasid_required(struct pci_dev *pdev);
> +
> +#else /* CONFIG_PCI_PASID && CONFIG_PCI_PRI */
> +
>  static inline int pci_prg_resp_pasid_required(struct pci_dev *pdev)
>  {
>  	return 0;
>  }
> -#endif /* CONFIG_PCI_PASID */
> -
> +#endif
>  
>  #endif /* LINUX_PCI_ATS_H*/
> -- 
> 2.21.0
> 

Re: [PATCH v7 1/7] PCI/ATS: Fix pci_prg_resp_pasid_required() dependency issues

[Kuppuswamy Sathyanarayanan]

Hi Bjorn,

Thanks for looking into this patch set.

On 9/5/19 12:18 PM, Bjorn Helgaas wrote:
> On Wed, Aug 28, 2019 at 03:14:01PM -0700, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
>> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>>
>> Since pci_prg_resp_pasid_required() function has dependency on both
>> PASID and PRI, define it only if both CONFIG_PCI_PRI and
>> CONFIG_PCI_PASID config options are enabled.
>>
>> Fixes: e5567f5f6762 ("PCI/ATS: Add pci_prg_resp_pasid_required()
>> interface.")
> [Don't split tags, including "Fixes:" across lines]
>
> This definitely doesn't fix e5567f5f6762.  That commit added
> pci_prg_resp_pasid_required(), but with no dependency on
> CONFIG_PCI_PRI or CONFIG_PCI_PASID.
>
> This patch is only required when a subsequent patch is applied.  It
> should be squashed into the commit that requires it so it's obvious
> why it's needed.
>
> I've been poking at this series, and I'll post a v8 soon with this and
> other fixes.
In your v8 submission you did not merge this patch. You did not use
pri_cap or pasid_cap cached values. Instead you have re-read the
value from register. Is this intentional?

Since this function will be called for every VF device we might loose 
some performance benefit.

>
>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>> ---
>>   drivers/pci/ats.c       | 10 ++++++----
>>   include/linux/pci-ats.h | 12 +++++++++---
>>   2 files changed, 15 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
>> index e18499243f84..cdd936d10f68 100644
>> --- a/drivers/pci/ats.c
>> +++ b/drivers/pci/ats.c
>> @@ -395,6 +395,8 @@ 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.
>> @@ -402,10 +404,8 @@ 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)
>>   {
>> @@ -425,6 +425,8 @@ int pci_prg_resp_pasid_required(struct pci_dev *pdev)
>>   }
>>   EXPORT_SYMBOL_GPL(pci_prg_resp_pasid_required);
>>   
>> +#endif
>> +
>>   #define PASID_NUMBER_SHIFT	8
>>   #define PASID_NUMBER_MASK	(0x1f << PASID_NUMBER_SHIFT)
>>   /**
>> diff --git a/include/linux/pci-ats.h b/include/linux/pci-ats.h
>> index 1ebb88e7c184..1a0bdaee2f32 100644
>> --- a/include/linux/pci-ats.h
>> +++ b/include/linux/pci-ats.h
>> @@ -40,7 +40,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 */
>>   
>> @@ -67,11 +66,18 @@ static inline int pci_max_pasids(struct pci_dev *pdev)
>>   	return -EINVAL;
>>   }
>>   
>> +#endif /* CONFIG_PCI_PASID */
>> +
>> +#if defined(CONFIG_PCI_PRI) && defined(CONFIG_PCI_PASID)
>> +
>> +int pci_prg_resp_pasid_required(struct pci_dev *pdev);
>> +
>> +#else /* CONFIG_PCI_PASID && CONFIG_PCI_PRI */
>> +
>>   static inline int pci_prg_resp_pasid_required(struct pci_dev *pdev)
>>   {
>>   	return 0;
>>   }
>> -#endif /* CONFIG_PCI_PASID */
>> -
>> +#endif
>>   
>>   #endif /* LINUX_PCI_ATS_H*/
>> -- 
>> 2.21.0
>>
-- 
Sathyanarayanan Kuppuswamy
Linux kernel developer

Re: [PATCH v7 1/7] PCI/ATS: Fix pci_prg_resp_pasid_required() dependency issues

[Bjorn Helgaas]

On Thu, Oct 03, 2019 at 10:20:24AM -0700, Kuppuswamy Sathyanarayanan wrote:
> Hi Bjorn,
> 
> Thanks for looking into this patch set.
> 
> On 9/5/19 12:18 PM, Bjorn Helgaas wrote:
> > On Wed, Aug 28, 2019 at 03:14:01PM -0700, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
> > > From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> > > 
> > > Since pci_prg_resp_pasid_required() function has dependency on both
> > > PASID and PRI, define it only if both CONFIG_PCI_PRI and
> > > CONFIG_PCI_PASID config options are enabled.
> > > 
> > > Fixes: e5567f5f6762 ("PCI/ATS: Add pci_prg_resp_pasid_required()
> > > interface.")
> > [Don't split tags, including "Fixes:" across lines]
> > 
> > This definitely doesn't fix e5567f5f6762.  That commit added
> > pci_prg_resp_pasid_required(), but with no dependency on
> > CONFIG_PCI_PRI or CONFIG_PCI_PASID.
> > 
> > This patch is only required when a subsequent patch is applied.  It
> > should be squashed into the commit that requires it so it's obvious
> > why it's needed.
> > 
> > I've been poking at this series, and I'll post a v8 soon with this and
> > other fixes.
> In your v8 submission you did not merge this patch. You did not use
> pri_cap or pasid_cap cached values. Instead you have re-read the
> value from register. Is this intentional?
> 
> Since this function will be called for every VF device we might loose some
> performance benefit.

This particular patch doesn't do any caching.  IIRC it fiddles with
ifdefs to solve a problem that would be introduced by a future patch.
I don't remember the exact details, but I think the series I merged
doesn't have that problem.  If it does, let me know the details and we
can fix it.

I did include the caching patches for both PRI and PASID capabilities,
but they're only performance optimizations so I moved them to the end
so the functional fixes would be smaller and earlier in the series.

> > > Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> > > ---
> > >   drivers/pci/ats.c       | 10 ++++++----
> > >   include/linux/pci-ats.h | 12 +++++++++---
> > >   2 files changed, 15 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
> > > index e18499243f84..cdd936d10f68 100644
> > > --- a/drivers/pci/ats.c
> > > +++ b/drivers/pci/ats.c
> > > @@ -395,6 +395,8 @@ 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.
> > > @@ -402,10 +404,8 @@ 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)
> > >   {
> > > @@ -425,6 +425,8 @@ int pci_prg_resp_pasid_required(struct pci_dev *pdev)
> > >   }
> > >   EXPORT_SYMBOL_GPL(pci_prg_resp_pasid_required);
> > > +#endif
> > > +
> > >   #define PASID_NUMBER_SHIFT	8
> > >   #define PASID_NUMBER_MASK	(0x1f << PASID_NUMBER_SHIFT)
> > >   /**
> > > diff --git a/include/linux/pci-ats.h b/include/linux/pci-ats.h
> > > index 1ebb88e7c184..1a0bdaee2f32 100644
> > > --- a/include/linux/pci-ats.h
> > > +++ b/include/linux/pci-ats.h
> > > @@ -40,7 +40,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 */
> > > @@ -67,11 +66,18 @@ static inline int pci_max_pasids(struct pci_dev *pdev)
> > >   	return -EINVAL;
> > >   }
> > > +#endif /* CONFIG_PCI_PASID */
> > > +
> > > +#if defined(CONFIG_PCI_PRI) && defined(CONFIG_PCI_PASID)
> > > +
> > > +int pci_prg_resp_pasid_required(struct pci_dev *pdev);
> > > +
> > > +#else /* CONFIG_PCI_PASID && CONFIG_PCI_PRI */
> > > +
> > >   static inline int pci_prg_resp_pasid_required(struct pci_dev *pdev)
> > >   {
> > >   	return 0;
> > >   }
> > > -#endif /* CONFIG_PCI_PASID */
> > > -
> > > +#endif
> > >   #endif /* LINUX_PCI_ATS_H*/
> > > -- 
> > > 2.21.0
> > > 
> -- 
> Sathyanarayanan Kuppuswamy
> Linux kernel developer
> 

Re: [PATCH v7 1/7] PCI/ATS: Fix pci_prg_resp_pasid_required() dependency issues

[Kuppuswamy Sathyanarayanan]

On Thu, Oct 03, 2019 at 02:04:13PM -0500, Bjorn Helgaas wrote:
> On Thu, Oct 03, 2019 at 10:20:24AM -0700, Kuppuswamy Sathyanarayanan wrote:
> > Hi Bjorn,
> > 
> > Thanks for looking into this patch set.
> > 
> > On 9/5/19 12:18 PM, Bjorn Helgaas wrote:
> > > On Wed, Aug 28, 2019 at 03:14:01PM -0700, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
> > > > From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> > > > 
> > > > Since pci_prg_resp_pasid_required() function has dependency on both
> > > > PASID and PRI, define it only if both CONFIG_PCI_PRI and
> > > > CONFIG_PCI_PASID config options are enabled.
> > > > 
> > > > Fixes: e5567f5f6762 ("PCI/ATS: Add pci_prg_resp_pasid_required()
> > > > interface.")
> > > [Don't split tags, including "Fixes:" across lines]
> > > 
> > > This definitely doesn't fix e5567f5f6762.  That commit added
> > > pci_prg_resp_pasid_required(), but with no dependency on
> > > CONFIG_PCI_PRI or CONFIG_PCI_PASID.
> > > 
> > > This patch is only required when a subsequent patch is applied.  It
> > > should be squashed into the commit that requires it so it's obvious
> > > why it's needed.
> > > 
> > > I've been poking at this series, and I'll post a v8 soon with this and
> > > other fixes.
> > In your v8 submission you did not merge this patch. You did not use
> > pri_cap or pasid_cap cached values. Instead you have re-read the
> > value from register. Is this intentional?
> > 
> > Since this function will be called for every VF device we might loose some
> > performance benefit.
> 
> This particular patch doesn't do any caching.  IIRC it fiddles with
> ifdefs to solve a problem that would be introduced by a future patch.
> I don't remember the exact details, but I think the series I merged
> doesn't have that problem.  If it does, let me know the details and we
> can fix it.
This patch by itself does not do any caching. But your caching patch
missed modifying this function to use cached values. Please check the
current implementation of this function. It still reads
PCI_EXT_CAP_ID_PRI register instead of using cached value. Please let
me know your comments.

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);

If caching is applied to this function then we need this #ifdef
dependency correction patch.

> 
> I did include the caching patches for both PRI and PASID capabilities,
> but they're only performance optimizations so I moved them to the end
> so the functional fixes would be smaller and earlier in the series.
> 
> > > > Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> > > > ---
> > > >   drivers/pci/ats.c       | 10 ++++++----
> > > >   include/linux/pci-ats.h | 12 +++++++++---
> > > >   2 files changed, 15 insertions(+), 7 deletions(-)
> > > > 
> > > > diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
> > > > index e18499243f84..cdd936d10f68 100644
> > > > --- a/drivers/pci/ats.c
> > > > +++ b/drivers/pci/ats.c
> > > > @@ -395,6 +395,8 @@ 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.
> > > > @@ -402,10 +404,8 @@ 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)
> > > >   {
> > > > @@ -425,6 +425,8 @@ int pci_prg_resp_pasid_required(struct pci_dev *pdev)
> > > >   }
> > > >   EXPORT_SYMBOL_GPL(pci_prg_resp_pasid_required);
> > > > +#endif
> > > > +
> > > >   #define PASID_NUMBER_SHIFT	8
> > > >   #define PASID_NUMBER_MASK	(0x1f << PASID_NUMBER_SHIFT)
> > > >   /**
> > > > diff --git a/include/linux/pci-ats.h b/include/linux/pci-ats.h
> > > > index 1ebb88e7c184..1a0bdaee2f32 100644
> > > > --- a/include/linux/pci-ats.h
> > > > +++ b/include/linux/pci-ats.h
> > > > @@ -40,7 +40,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 */
> > > > @@ -67,11 +66,18 @@ static inline int pci_max_pasids(struct pci_dev *pdev)
> > > >   	return -EINVAL;
> > > >   }
> > > > +#endif /* CONFIG_PCI_PASID */
> > > > +
> > > > +#if defined(CONFIG_PCI_PRI) && defined(CONFIG_PCI_PASID)
> > > > +
> > > > +int pci_prg_resp_pasid_required(struct pci_dev *pdev);
> > > > +
> > > > +#else /* CONFIG_PCI_PASID && CONFIG_PCI_PRI */
> > > > +
> > > >   static inline int pci_prg_resp_pasid_required(struct pci_dev *pdev)
> > > >   {
> > > >   	return 0;
> > > >   }
> > > > -#endif /* CONFIG_PCI_PASID */
> > > > -
> > > > +#endif
> > > >   #endif /* LINUX_PCI_ATS_H*/
> > > > -- 
> > > > 2.21.0
> > > > 
> > -- 
> > Sathyanarayanan Kuppuswamy
> > Linux kernel developer
> > 

-- 
Sathyanarayanan Kuppuswamy
Linux kernel developer

Re: [PATCH v7 1/7] PCI/ATS: Fix pci_prg_resp_pasid_required() dependency issues

[Bjorn Helgaas]

On Thu, Oct 03, 2019 at 01:37:26PM -0700, Kuppuswamy Sathyanarayanan wrote:
> On Thu, Oct 03, 2019 at 02:04:13PM -0500, Bjorn Helgaas wrote:
> > On Thu, Oct 03, 2019 at 10:20:24AM -0700, Kuppuswamy Sathyanarayanan wrote:
> > > Hi Bjorn,
> > > 
> > > Thanks for looking into this patch set.
> > > 
> > > On 9/5/19 12:18 PM, Bjorn Helgaas wrote:
> > > > On Wed, Aug 28, 2019 at 03:14:01PM -0700, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
> > > > > From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> > > > > 
> > > > > Since pci_prg_resp_pasid_required() function has dependency on both
> > > > > PASID and PRI, define it only if both CONFIG_PCI_PRI and
> > > > > CONFIG_PCI_PASID config options are enabled.
> > > > > 
> > > > > Fixes: e5567f5f6762 ("PCI/ATS: Add pci_prg_resp_pasid_required()
> > > > > interface.")
> > > > [Don't split tags, including "Fixes:" across lines]
> > > > 
> > > > This definitely doesn't fix e5567f5f6762.  That commit added
> > > > pci_prg_resp_pasid_required(), but with no dependency on
> > > > CONFIG_PCI_PRI or CONFIG_PCI_PASID.
> > > > 
> > > > This patch is only required when a subsequent patch is applied.  It
> > > > should be squashed into the commit that requires it so it's obvious
> > > > why it's needed.
> > > > 
> > > > I've been poking at this series, and I'll post a v8 soon with this and
> > > > other fixes.
> > > In your v8 submission you did not merge this patch. You did not use
> > > pri_cap or pasid_cap cached values. Instead you have re-read the
> > > value from register. Is this intentional?
> > > 
> > > Since this function will be called for every VF device we might loose some
> > > performance benefit.
> > 
> > This particular patch doesn't do any caching.  IIRC it fiddles with
> > ifdefs to solve a problem that would be introduced by a future patch.
> > I don't remember the exact details, but I think the series I merged
> > doesn't have that problem.  If it does, let me know the details and we
> > can fix it.
> This patch by itself does not do any caching. But your caching patch
> missed modifying this function to use cached values. Please check the
> current implementation of this function. It still reads
> PCI_EXT_CAP_ID_PRI register instead of using cached value. Please let
> me know your comments.
> 
> 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);
> 
> If caching is applied to this function then we need this #ifdef
> dependency correction patch.

IIRC this #ifdef patch wasn't connected to the actual *need* for the
#ifdef, so it was very difficult to review.  I thought this function
would be infrequently used and it wasn't worth trying to sort out the
#ifdef muddle to do the caching.  But it does seem sort of pointless
to chase the capability list again here, so maybe it *is* worth
optimizing.

The PRG Response PASID Required bit is read-only, so I wonder if it
would be simpler if we just read PCI_PRI_STATUS once and save the bit
in the struct pci_dev?  We could do that in pci_enable_pri(), or if we
might need the value before that's called, we could add a
pci_pri_init() and do it there.

> > I did include the caching patches for both PRI and PASID capabilities,
> > but they're only performance optimizations so I moved them to the end
> > so the functional fixes would be smaller and earlier in the series.
> > 
> > > > > Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> > > > > ---
> > > > >   drivers/pci/ats.c       | 10 ++++++----
> > > > >   include/linux/pci-ats.h | 12 +++++++++---
> > > > >   2 files changed, 15 insertions(+), 7 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
> > > > > index e18499243f84..cdd936d10f68 100644
> > > > > --- a/drivers/pci/ats.c
> > > > > +++ b/drivers/pci/ats.c
> > > > > @@ -395,6 +395,8 @@ 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.
> > > > > @@ -402,10 +404,8 @@ 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)
> > > > >   {
> > > > > @@ -425,6 +425,8 @@ int pci_prg_resp_pasid_required(struct pci_dev *pdev)
> > > > >   }
> > > > >   EXPORT_SYMBOL_GPL(pci_prg_resp_pasid_required);
> > > > > +#endif
> > > > > +
> > > > >   #define PASID_NUMBER_SHIFT	8
> > > > >   #define PASID_NUMBER_MASK	(0x1f << PASID_NUMBER_SHIFT)
> > > > >   /**
> > > > > diff --git a/include/linux/pci-ats.h b/include/linux/pci-ats.h
> > > > > index 1ebb88e7c184..1a0bdaee2f32 100644
> > > > > --- a/include/linux/pci-ats.h
> > > > > +++ b/include/linux/pci-ats.h
> > > > > @@ -40,7 +40,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 */
> > > > > @@ -67,11 +66,18 @@ static inline int pci_max_pasids(struct pci_dev *pdev)
> > > > >   	return -EINVAL;
> > > > >   }
> > > > > +#endif /* CONFIG_PCI_PASID */
> > > > > +
> > > > > +#if defined(CONFIG_PCI_PRI) && defined(CONFIG_PCI_PASID)
> > > > > +
> > > > > +int pci_prg_resp_pasid_required(struct pci_dev *pdev);
> > > > > +
> > > > > +#else /* CONFIG_PCI_PASID && CONFIG_PCI_PRI */
> > > > > +
> > > > >   static inline int pci_prg_resp_pasid_required(struct pci_dev *pdev)
> > > > >   {
> > > > >   	return 0;
> > > > >   }
> > > > > -#endif /* CONFIG_PCI_PASID */
> > > > > -
> > > > > +#endif
> > > > >   #endif /* LINUX_PCI_ATS_H*/
> > > > > -- 
> > > > > 2.21.0
> > > > > 
> > > -- 
> > > Sathyanarayanan Kuppuswamy
> > > Linux kernel developer
> > > 
> 
> -- 
> Sathyanarayanan Kuppuswamy
> Linux kernel developer

Re: [PATCH v7 1/7] PCI/ATS: Fix pci_prg_resp_pasid_required() dependency issues

[Kuppuswamy Sathyanarayanan]

On 10/3/19 2:01 PM, Bjorn Helgaas wrote:
> On Thu, Oct 03, 2019 at 01:37:26PM -0700, Kuppuswamy Sathyanarayanan wrote:
>> On Thu, Oct 03, 2019 at 02:04:13PM -0500, Bjorn Helgaas wrote:
>>> On Thu, Oct 03, 2019 at 10:20:24AM -0700, Kuppuswamy Sathyanarayanan wrote:
>>>> Hi Bjorn,
>>>>
>>>> Thanks for looking into this patch set.
>>>>
>>>> On 9/5/19 12:18 PM, Bjorn Helgaas wrote:
>>>>> On Wed, Aug 28, 2019 at 03:14:01PM -0700, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
>>>>>> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>>>>>>
>>>>>> Since pci_prg_resp_pasid_required() function has dependency on both
>>>>>> PASID and PRI, define it only if both CONFIG_PCI_PRI and
>>>>>> CONFIG_PCI_PASID config options are enabled.
>>>>>>
>>>>>> Fixes: e5567f5f6762 ("PCI/ATS: Add pci_prg_resp_pasid_required()
>>>>>> interface.")
>>>>> [Don't split tags, including "Fixes:" across lines]
>>>>>
>>>>> This definitely doesn't fix e5567f5f6762.  That commit added
>>>>> pci_prg_resp_pasid_required(), but with no dependency on
>>>>> CONFIG_PCI_PRI or CONFIG_PCI_PASID.
>>>>>
>>>>> This patch is only required when a subsequent patch is applied.  It
>>>>> should be squashed into the commit that requires it so it's obvious
>>>>> why it's needed.
>>>>>
>>>>> I've been poking at this series, and I'll post a v8 soon with this and
>>>>> other fixes.
>>>> In your v8 submission you did not merge this patch. You did not use
>>>> pri_cap or pasid_cap cached values. Instead you have re-read the
>>>> value from register. Is this intentional?
>>>>
>>>> Since this function will be called for every VF device we might loose some
>>>> performance benefit.
>>> This particular patch doesn't do any caching.  IIRC it fiddles with
>>> ifdefs to solve a problem that would be introduced by a future patch.
>>> I don't remember the exact details, but I think the series I merged
>>> doesn't have that problem.  If it does, let me know the details and we
>>> can fix it.
>> This patch by itself does not do any caching. But your caching patch
>> missed modifying this function to use cached values. Please check the
>> current implementation of this function. It still reads
>> PCI_EXT_CAP_ID_PRI register instead of using cached value. Please let
>> me know your comments.
>>
>> 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);
>>
>> If caching is applied to this function then we need this #ifdef
>> dependency correction patch.
> IIRC this #ifdef patch wasn't connected to the actual *need* for the
> #ifdef, so it was very difficult to review.  I thought this function
> would be infrequently used and it wasn't worth trying to sort out the
> #ifdef muddle to do the caching.  But it does seem sort of pointless
> to chase the capability list again here, so maybe it *is* worth
> optimizing.
>
> The PRG Response PASID Required bit is read-only, so I wonder if it
> would be simpler if we just read PCI_PRI_STATUS once and save the bit
> in the struct pci_dev?  We could do that in pci_enable_pri(), or if we
> might need the value before that's called, we could add a
> pci_pri_init() and do it there.

Yes, caching PASID Required bit in pci_pri_init() function would provide 
performance
benefits. But another thing to consider is, since this bit is same for 
both PF/VF, is it worth to
add this bit it to struct pci_dev?or struct pci_sriov is the more 
appropriate place?

>
>>> I did include the caching patches for both PRI and PASID capabilities,
>>> but they're only performance optimizations so I moved them to the end
>>> so the functional fixes would be smaller and earlier in the series.
>>>
>>>>>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>>>>>> ---
>>>>>>    drivers/pci/ats.c       | 10 ++++++----
>>>>>>    include/linux/pci-ats.h | 12 +++++++++---
>>>>>>    2 files changed, 15 insertions(+), 7 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
>>>>>> index e18499243f84..cdd936d10f68 100644
>>>>>> --- a/drivers/pci/ats.c
>>>>>> +++ b/drivers/pci/ats.c
>>>>>> @@ -395,6 +395,8 @@ 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.
>>>>>> @@ -402,10 +404,8 @@ 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)
>>>>>>    {
>>>>>> @@ -425,6 +425,8 @@ int pci_prg_resp_pasid_required(struct pci_dev *pdev)
>>>>>>    }
>>>>>>    EXPORT_SYMBOL_GPL(pci_prg_resp_pasid_required);
>>>>>> +#endif
>>>>>> +
>>>>>>    #define PASID_NUMBER_SHIFT	8
>>>>>>    #define PASID_NUMBER_MASK	(0x1f << PASID_NUMBER_SHIFT)
>>>>>>    /**
>>>>>> diff --git a/include/linux/pci-ats.h b/include/linux/pci-ats.h
>>>>>> index 1ebb88e7c184..1a0bdaee2f32 100644
>>>>>> --- a/include/linux/pci-ats.h
>>>>>> +++ b/include/linux/pci-ats.h
>>>>>> @@ -40,7 +40,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 */
>>>>>> @@ -67,11 +66,18 @@ static inline int pci_max_pasids(struct pci_dev *pdev)
>>>>>>    	return -EINVAL;
>>>>>>    }
>>>>>> +#endif /* CONFIG_PCI_PASID */
>>>>>> +
>>>>>> +#if defined(CONFIG_PCI_PRI) && defined(CONFIG_PCI_PASID)
>>>>>> +
>>>>>> +int pci_prg_resp_pasid_required(struct pci_dev *pdev);
>>>>>> +
>>>>>> +#else /* CONFIG_PCI_PASID && CONFIG_PCI_PRI */
>>>>>> +
>>>>>>    static inline int pci_prg_resp_pasid_required(struct pci_dev *pdev)
>>>>>>    {
>>>>>>    	return 0;
>>>>>>    }
>>>>>> -#endif /* CONFIG_PCI_PASID */
>>>>>> -
>>>>>> +#endif
>>>>>>    #endif /* LINUX_PCI_ATS_H*/
>>>>>> -- 
>>>>>> 2.21.0
>>>>>>
>>>> -- 
>>>> Sathyanarayanan Kuppuswamy
>>>> Linux kernel developer
>>>>
>> -- 
>> Sathyanarayanan Kuppuswamy
>> Linux kernel developer

-- 
Sathyanarayanan Kuppuswamy
Linux kernel developer

Re: [PATCH v7 1/7] PCI/ATS: Fix pci_prg_resp_pasid_required() dependency issues

[Bjorn Helgaas]

On Thu, Oct 03, 2019 at 02:11:28PM -0700, Kuppuswamy Sathyanarayanan wrote:
> 
> On 10/3/19 2:01 PM, Bjorn Helgaas wrote:
> > On Thu, Oct 03, 2019 at 01:37:26PM -0700, Kuppuswamy Sathyanarayanan wrote:
> > > On Thu, Oct 03, 2019 at 02:04:13PM -0500, Bjorn Helgaas wrote:
> > > > On Thu, Oct 03, 2019 at 10:20:24AM -0700, Kuppuswamy Sathyanarayanan wrote:
> > > > > Hi Bjorn,
> > > > > 
> > > > > Thanks for looking into this patch set.
> > > > > 
> > > > > On 9/5/19 12:18 PM, Bjorn Helgaas wrote:
> > > > > > On Wed, Aug 28, 2019 at 03:14:01PM -0700, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
> > > > > > > From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> > > > > > > 
> > > > > > > Since pci_prg_resp_pasid_required() function has dependency on both
> > > > > > > PASID and PRI, define it only if both CONFIG_PCI_PRI and
> > > > > > > CONFIG_PCI_PASID config options are enabled.
> > > > > > > 
> > > > > > > Fixes: e5567f5f6762 ("PCI/ATS: Add pci_prg_resp_pasid_required()
> > > > > > > interface.")
> > > > > > [Don't split tags, including "Fixes:" across lines]
> > > > > > 
> > > > > > This definitely doesn't fix e5567f5f6762.  That commit added
> > > > > > pci_prg_resp_pasid_required(), but with no dependency on
> > > > > > CONFIG_PCI_PRI or CONFIG_PCI_PASID.
> > > > > > 
> > > > > > This patch is only required when a subsequent patch is applied.  It
> > > > > > should be squashed into the commit that requires it so it's obvious
> > > > > > why it's needed.
> > > > > > 
> > > > > > I've been poking at this series, and I'll post a v8 soon with this and
> > > > > > other fixes.
> > > > > In your v8 submission you did not merge this patch. You did not use
> > > > > pri_cap or pasid_cap cached values. Instead you have re-read the
> > > > > value from register. Is this intentional?
> > > > > 
> > > > > Since this function will be called for every VF device we might loose some
> > > > > performance benefit.
> > > > This particular patch doesn't do any caching.  IIRC it fiddles with
> > > > ifdefs to solve a problem that would be introduced by a future patch.
> > > > I don't remember the exact details, but I think the series I merged
> > > > doesn't have that problem.  If it does, let me know the details and we
> > > > can fix it.
> > > This patch by itself does not do any caching. But your caching patch
> > > missed modifying this function to use cached values. Please check the
> > > current implementation of this function. It still reads
> > > PCI_EXT_CAP_ID_PRI register instead of using cached value. Please let
> > > me know your comments.
> > > 
> > > 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);
> > > 
> > > If caching is applied to this function then we need this #ifdef
> > > dependency correction patch.
> > IIRC this #ifdef patch wasn't connected to the actual *need* for the
> > #ifdef, so it was very difficult to review.  I thought this function
> > would be infrequently used and it wasn't worth trying to sort out the
> > #ifdef muddle to do the caching.  But it does seem sort of pointless
> > to chase the capability list again here, so maybe it *is* worth
> > optimizing.
> > 
> > The PRG Response PASID Required bit is read-only, so I wonder if it
> > would be simpler if we just read PCI_PRI_STATUS once and save the bit
> > in the struct pci_dev?  We could do that in pci_enable_pri(), or if we
> > might need the value before that's called, we could add a
> > pci_pri_init() and do it there.
> 
> Yes, caching PASID Required bit in pci_pri_init() function would
> provide performance benefits. But another thing to consider is,
> since this bit is same for both PF/VF, is it worth to add this bit
> it to struct pci_dev?or struct pci_sriov is the more appropriate
> place?

IIUC, the PRI capability is not specific to SR-IOV, so I don't think
it would make sense to cache PRG Response PASID Required in pci_sriov.

PFs may implement PRI; VFs do not (PCIe r5.0, sec 10.5.2), so I think
the bit should be cached in the pci_dev, and if we want to know the
value for a VF, we should read it from the PF's pci_dev.

Bjorn

[PATCH v7 2/7] PCI/ATS: Cache PRI capability check result

[sathyanarayanan.kuppuswamy]

From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

Currently, PRI capability checks are repeated across all PRI API's.
Instead, cache the capability check result in pci_pri_init() and use it
in other PRI API's.

Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 drivers/pci/ats.c   | 56 +++++++++++++++++++++++++--------------------
 include/linux/pci.h |  1 +
 2 files changed, 32 insertions(+), 25 deletions(-)

diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
index cdd936d10f68..b863562b6702 100644
--- a/drivers/pci/ats.c
+++ b/drivers/pci/ats.c
@@ -16,6 +16,19 @@
 
 #include "pci.h"
 
+static void pci_pri_init(struct pci_dev *pdev)
+{
+#ifdef CONFIG_PCI_PRI
+	int pos;
+
+	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_PRI);
+	if (!pos)
+		return;
+
+	pdev->pri_cap = pos;
+#endif
+}
+
 void pci_ats_init(struct pci_dev *dev)
 {
 	int pos;
@@ -28,6 +41,8 @@ void pci_ats_init(struct pci_dev *dev)
 		return;
 
 	dev->ats_cap = pos;
+
+	pci_pri_init(dev);
 }
 
 /**
@@ -180,26 +195,25 @@ int pci_enable_pri(struct pci_dev *pdev, u32 reqs)
 {
 	u16 control, status;
 	u32 max_requests;
-	int pos;
 
 	if (WARN_ON(pdev->pri_enabled))
 		return -EBUSY;
 
-	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_PRI);
-	if (!pos)
+	if (!pdev->pri_cap)
 		return -EINVAL;
 
-	pci_read_config_word(pdev, pos + PCI_PRI_STATUS, &status);
+	pci_read_config_word(pdev, pdev->pri_cap + PCI_PRI_STATUS, &status);
 	if (!(status & PCI_PRI_STATUS_STOPPED))
 		return -EBUSY;
 
-	pci_read_config_dword(pdev, pos + PCI_PRI_MAX_REQ, &max_requests);
+	pci_read_config_dword(pdev, pdev->pri_cap + PCI_PRI_MAX_REQ,
+			      &max_requests);
 	reqs = min(max_requests, reqs);
 	pdev->pri_reqs_alloc = reqs;
-	pci_write_config_dword(pdev, pos + PCI_PRI_ALLOC_REQ, reqs);
+	pci_write_config_dword(pdev, pdev->pri_cap + PCI_PRI_ALLOC_REQ, reqs);
 
 	control = PCI_PRI_CTRL_ENABLE;
-	pci_write_config_word(pdev, pos + PCI_PRI_CTRL, control);
+	pci_write_config_word(pdev, pdev->pri_cap + PCI_PRI_CTRL, control);
 
 	pdev->pri_enabled = 1;
 
@@ -216,18 +230,16 @@ EXPORT_SYMBOL_GPL(pci_enable_pri);
 void pci_disable_pri(struct pci_dev *pdev)
 {
 	u16 control;
-	int pos;
 
 	if (WARN_ON(!pdev->pri_enabled))
 		return;
 
-	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_PRI);
-	if (!pos)
+	if (!pdev->pri_cap)
 		return;
 
-	pci_read_config_word(pdev, pos + PCI_PRI_CTRL, &control);
+	pci_read_config_word(pdev, pdev->pri_cap + PCI_PRI_CTRL, &control);
 	control &= ~PCI_PRI_CTRL_ENABLE;
-	pci_write_config_word(pdev, pos + PCI_PRI_CTRL, control);
+	pci_write_config_word(pdev, pdev->pri_cap + PCI_PRI_CTRL, control);
 
 	pdev->pri_enabled = 0;
 }
@@ -241,17 +253,15 @@ void pci_restore_pri_state(struct pci_dev *pdev)
 {
 	u16 control = PCI_PRI_CTRL_ENABLE;
 	u32 reqs = pdev->pri_reqs_alloc;
-	int pos;
 
 	if (!pdev->pri_enabled)
 		return;
 
-	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_PRI);
-	if (!pos)
+	if (!pdev->pri_cap)
 		return;
 
-	pci_write_config_dword(pdev, pos + PCI_PRI_ALLOC_REQ, reqs);
-	pci_write_config_word(pdev, pos + PCI_PRI_CTRL, control);
+	pci_write_config_dword(pdev, pdev->pri_cap + PCI_PRI_ALLOC_REQ, reqs);
+	pci_write_config_word(pdev, pdev->pri_cap + PCI_PRI_CTRL, control);
 }
 EXPORT_SYMBOL_GPL(pci_restore_pri_state);
 
@@ -265,17 +275,15 @@ EXPORT_SYMBOL_GPL(pci_restore_pri_state);
 int pci_reset_pri(struct pci_dev *pdev)
 {
 	u16 control;
-	int pos;
 
 	if (WARN_ON(pdev->pri_enabled))
 		return -EBUSY;
 
-	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_PRI);
-	if (!pos)
+	if (!pdev->pri_cap)
 		return -EINVAL;
 
 	control = PCI_PRI_CTRL_RESET;
-	pci_write_config_word(pdev, pos + PCI_PRI_CTRL, control);
+	pci_write_config_word(pdev, pdev->pri_cap + PCI_PRI_CTRL, control);
 
 	return 0;
 }
@@ -410,13 +418,11 @@ EXPORT_SYMBOL_GPL(pci_pasid_features);
 int pci_prg_resp_pasid_required(struct pci_dev *pdev)
 {
 	u16 status;
-	int pos;
 
-	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_PRI);
-	if (!pos)
+	if (!pdev->pri_cap)
 		return 0;
 
-	pci_read_config_word(pdev, pos + PCI_PRI_STATUS, &status);
+	pci_read_config_word(pdev, pdev->pri_cap + PCI_PRI_STATUS, &status);
 
 	if (status & PCI_PRI_STATUS_PASID)
 		return 1;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 82e4cd1b7ac3..79991984c4cd 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -455,6 +455,7 @@ struct pci_dev {
 	atomic_t	ats_ref_cnt;	/* Number of VFs with ATS enabled */
 #endif
 #ifdef CONFIG_PCI_PRI
+	u16		pri_cap;	/* PRI Capability offset */
 	u32		pri_reqs_alloc; /* Number of PRI requests allocated */
 #endif
 #ifdef CONFIG_PCI_PASID
-- 
2.21.0

[PATCH v7 3/7] PCI/ATS: Cache PASID capability check result

[sathyanarayanan.kuppuswamy]

From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

Currently, PASID capability checks are repeated across all PASID API's.
Instead, cache the capability check result in pci_pasid_init() and use
it in other PASID API's.

Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 drivers/pci/ats.c   | 50 ++++++++++++++++++++++++++-------------------
 include/linux/pci.h |  1 +
 2 files changed, 30 insertions(+), 21 deletions(-)

diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
index b863562b6702..022698a85c98 100644
--- a/drivers/pci/ats.c
+++ b/drivers/pci/ats.c
@@ -29,6 +29,19 @@ static void pci_pri_init(struct pci_dev *pdev)
 #endif
 }
 
+static void pci_pasid_init(struct pci_dev *pdev)
+{
+#ifdef CONFIG_PCI_PASID
+	int pos;
+
+	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_PASID);
+	if (!pos)
+		return;
+
+	pdev->pasid_cap = pos;
+#endif
+}
+
 void pci_ats_init(struct pci_dev *dev)
 {
 	int pos;
@@ -43,6 +56,8 @@ void pci_ats_init(struct pci_dev *dev)
 	dev->ats_cap = pos;
 
 	pci_pri_init(dev);
+
+	pci_pasid_init(dev);
 }
 
 /**
@@ -303,7 +318,6 @@ EXPORT_SYMBOL_GPL(pci_reset_pri);
 int pci_enable_pasid(struct pci_dev *pdev, int features)
 {
 	u16 control, supported;
-	int pos;
 
 	if (WARN_ON(pdev->pasid_enabled))
 		return -EBUSY;
@@ -311,11 +325,11 @@ int pci_enable_pasid(struct pci_dev *pdev, int features)
 	if (!pdev->eetlp_prefix_path)
 		return -EINVAL;
 
-	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_PASID);
-	if (!pos)
+	if (!pdev->pasid_cap)
 		return -EINVAL;
 
-	pci_read_config_word(pdev, pos + PCI_PASID_CAP, &supported);
+	pci_read_config_word(pdev, pdev->pasid_cap + PCI_PASID_CAP,
+			     &supported);
 	supported &= PCI_PASID_CAP_EXEC | PCI_PASID_CAP_PRIV;
 
 	/* User wants to enable anything unsupported? */
@@ -325,7 +339,7 @@ int pci_enable_pasid(struct pci_dev *pdev, int features)
 	control = PCI_PASID_CTRL_ENABLE | features;
 	pdev->pasid_features = features;
 
-	pci_write_config_word(pdev, pos + PCI_PASID_CTRL, control);
+	pci_write_config_word(pdev, pdev->pasid_cap + PCI_PASID_CTRL, control);
 
 	pdev->pasid_enabled = 1;
 
@@ -340,16 +354,14 @@ EXPORT_SYMBOL_GPL(pci_enable_pasid);
 void pci_disable_pasid(struct pci_dev *pdev)
 {
 	u16 control = 0;
-	int pos;
 
 	if (WARN_ON(!pdev->pasid_enabled))
 		return;
 
-	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_PASID);
-	if (!pos)
+	if (!pdev->pasid_cap)
 		return;
 
-	pci_write_config_word(pdev, pos + PCI_PASID_CTRL, control);
+	pci_write_config_word(pdev, pdev->pasid_cap + PCI_PASID_CTRL, control);
 
 	pdev->pasid_enabled = 0;
 }
@@ -362,17 +374,15 @@ EXPORT_SYMBOL_GPL(pci_disable_pasid);
 void pci_restore_pasid_state(struct pci_dev *pdev)
 {
 	u16 control;
-	int pos;
 
 	if (!pdev->pasid_enabled)
 		return;
 
-	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_PASID);
-	if (!pos)
+	if (!pdev->pasid_cap)
 		return;
 
 	control = PCI_PASID_CTRL_ENABLE | pdev->pasid_features;
-	pci_write_config_word(pdev, pos + PCI_PASID_CTRL, control);
+	pci_write_config_word(pdev, pdev->pasid_cap + PCI_PASID_CTRL, control);
 }
 EXPORT_SYMBOL_GPL(pci_restore_pasid_state);
 
@@ -389,13 +399,12 @@ EXPORT_SYMBOL_GPL(pci_restore_pasid_state);
 int pci_pasid_features(struct pci_dev *pdev)
 {
 	u16 supported;
-	int pos;
 
-	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_PASID);
-	if (!pos)
+	if (!pdev->pasid_cap)
 		return -EINVAL;
 
-	pci_read_config_word(pdev, pos + PCI_PASID_CAP, &supported);
+	pci_read_config_word(pdev, pdev->pasid_cap + PCI_PASID_CAP,
+			     &supported);
 
 	supported &= PCI_PASID_CAP_EXEC | PCI_PASID_CAP_PRIV;
 
@@ -445,13 +454,12 @@ EXPORT_SYMBOL_GPL(pci_prg_resp_pasid_required);
 int pci_max_pasids(struct pci_dev *pdev)
 {
 	u16 supported;
-	int pos;
 
-	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_PASID);
-	if (!pos)
+	if (!pdev->pasid_cap)
 		return -EINVAL;
 
-	pci_read_config_word(pdev, pos + PCI_PASID_CAP, &supported);
+	pci_read_config_word(pdev, pdev->pasid_cap + PCI_PASID_CAP,
+			     &supported);
 
 	supported = (supported & PASID_NUMBER_MASK) >> PASID_NUMBER_SHIFT;
 
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 79991984c4cd..782bca667010 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -459,6 +459,7 @@ struct pci_dev {
 	u32		pri_reqs_alloc; /* Number of PRI requests allocated */
 #endif
 #ifdef CONFIG_PCI_PASID
+	u16		pasid_cap;	/* PASID Capability offset */
 	u16		pasid_features;
 #endif
 #ifdef CONFIG_PCI_P2PDMA
-- 
2.21.0

[PATCH v7 4/7] PCI/ATS: Add PRI support for PCIe VF devices

[sathyanarayanan.kuppuswamy]

From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

When IOMMU tries to enable Page Request Interface (PRI) for VF device
in iommu_enable_dev_iotlb(), it always fails because PRI support for
PCIe VF device is currently broken. Current implementation expects
the given PCIe device (PF & VF) to implement PRI capability before
enabling the PRI support. But this assumption is incorrect. As per PCIe
spec r4.0, sec 9.3.7.11, all VFs associated with PF can only use the
PRI of the PF and not implement it. Hence we need to create exception
for handling the PRI support for PCIe VF device.

Cc: Ashok Raj <ashok.raj@intel.com>
Cc: Keith Busch <keith.busch@intel.com>
Suggested-by: Ashok Raj <ashok.raj@intel.com>
Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 drivers/pci/ats.c | 42 ++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 40 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
index 022698a85c98..9af1e518a9ab 100644
--- a/drivers/pci/ats.c
+++ b/drivers/pci/ats.c
@@ -21,6 +21,15 @@ static void pci_pri_init(struct pci_dev *pdev)
 #ifdef CONFIG_PCI_PRI
 	int pos;
 
+	/*
+	 * As per PCIe r4.0, sec 9.3.7.11, only PF is permitted to
+	 * implement PRI and all associated VFs can only use it.
+	 * Since PF already initialized the PRI parameters there is
+	 * no need to proceed further.
+	 */
+	if (pdev->is_virtfn)
+		return;
+
 	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_PRI);
 	if (!pos)
 		return;
@@ -210,6 +219,20 @@ int pci_enable_pri(struct pci_dev *pdev, u32 reqs)
 {
 	u16 control, status;
 	u32 max_requests;
+	struct pci_dev *pf = pci_physfn(pdev);
+
+	/*
+	 * IOMMU is the only user of this function and as per
+	 * current usage, PF PRI enable always happens before
+	 * VF and hence we don't need to do anything special
+	 * for VF. So just return success if PRI is enabled in PF.
+	 */
+	if (pdev->is_virtfn) {
+		if (pf->pri_enabled)
+			return 0;
+		else
+			return -EINVAL;
+	}
 
 	if (WARN_ON(pdev->pri_enabled))
 		return -EBUSY;
@@ -246,6 +269,14 @@ void pci_disable_pri(struct pci_dev *pdev)
 {
 	u16 control;
 
+	/*
+	 * As per PCIe r4.0, sec 9.3.7.11, only PF is permitted to
+	 * implement PRI and all associated VFs can only use it.
+	 * So don't do anything for VF and just return.
+	 */
+	if (pdev->is_virtfn)
+		return;
+
 	if (WARN_ON(!pdev->pri_enabled))
 		return;
 
@@ -269,6 +300,9 @@ void pci_restore_pri_state(struct pci_dev *pdev)
 	u16 control = PCI_PRI_CTRL_ENABLE;
 	u32 reqs = pdev->pri_reqs_alloc;
 
+	if (pdev->is_virtfn)
+		return;
+
 	if (!pdev->pri_enabled)
 		return;
 
@@ -291,6 +325,9 @@ int pci_reset_pri(struct pci_dev *pdev)
 {
 	u16 control;
 
+	if (pdev->is_virtfn)
+		return 0;
+
 	if (WARN_ON(pdev->pri_enabled))
 		return -EBUSY;
 
@@ -427,11 +464,12 @@ EXPORT_SYMBOL_GPL(pci_pasid_features);
 int pci_prg_resp_pasid_required(struct pci_dev *pdev)
 {
 	u16 status;
+	struct pci_dev *pf = pci_physfn(pdev);
 
-	if (!pdev->pri_cap)
+	if (!pf->pri_cap)
 		return 0;
 
-	pci_read_config_word(pdev, pdev->pri_cap + PCI_PRI_STATUS, &status);
+	pci_read_config_word(pf, pf->pri_cap + PCI_PRI_STATUS, &status);
 
 	if (status & PCI_PRI_STATUS_PASID)
 		return 1;
-- 
2.21.0

[PATCH v7 5/7] PCI/ATS: Add PASID support for PCIe VF devices

[sathyanarayanan.kuppuswamy]

From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

When IOMMU tries to enable PASID for VF device in
iommu_enable_dev_iotlb(), it always fails because PASID support for PCIe
VF device is currently broken in PCIE driver. Current implementation
expects the given PCIe device (PF & VF) to implement PASID capability
before enabling the PASID support. But this assumption is incorrect. As
per PCIe spec r4.0, sec 9.3.7.14, all VFs associated with PF can only
use the PASID of the PF and not implement it. Hence we need to create
exception for handling the PASID support for PCIe VF device.

Cc: Ashok Raj <ashok.raj@intel.com>
Cc: Keith Busch <keith.busch@intel.com>
Suggested-by: Ashok Raj <ashok.raj@intel.com>
Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 drivers/pci/ats.c | 49 +++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 43 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
index 9af1e518a9ab..893674520bbf 100644
--- a/drivers/pci/ats.c
+++ b/drivers/pci/ats.c
@@ -43,6 +43,15 @@ static void pci_pasid_init(struct pci_dev *pdev)
 #ifdef CONFIG_PCI_PASID
 	int pos;
 
+	/*
+	 * As per PCIe r4.0, sec 9.3.7.14, only PF is permitted to
+	 * implement PASID Capability and all associated VFs can
+	 * only use it. Since PF already initialized the PASID
+	 * parameters there is no need to proceed further.
+	 */
+	if (pdev->is_virtfn)
+		return;
+
 	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_PASID);
 	if (!pos)
 		return;
@@ -355,7 +364,20 @@ EXPORT_SYMBOL_GPL(pci_reset_pri);
 int pci_enable_pasid(struct pci_dev *pdev, int features)
 {
 	u16 control, supported;
+	struct pci_dev *pf = pci_physfn(pdev);
 
+	/*
+	 * IOMMU is the only user of this function and as per
+	 * current usage, PF PASID enable always happens before
+	 * VF and hence we don't need to do anything special
+	 * for VF. So just return success if PASID is enabled in PF.
+	 */
+	if (pdev->is_virtfn) {
+		if (pf->pasid_enabled)
+			return 0;
+		else
+			return -EINVAL;
+	}
 	if (WARN_ON(pdev->pasid_enabled))
 		return -EBUSY;
 
@@ -392,6 +414,14 @@ void pci_disable_pasid(struct pci_dev *pdev)
 {
 	u16 control = 0;
 
+	/*
+	 * As per PCIe r4.0, sec 9.3.7.14, only PF is permitted to
+	 * implement PASID Capability and all associated VFs can
+	 * only use it. So don't do anything for VF and just return.
+	 */
+	if (pdev->is_virtfn)
+		return;
+
 	if (WARN_ON(!pdev->pasid_enabled))
 		return;
 
@@ -412,6 +442,13 @@ void pci_restore_pasid_state(struct pci_dev *pdev)
 {
 	u16 control;
 
+	/*
+	 * PF should have already restored the PASID state. So for
+	 * VF, just return.
+	 */
+	if (pdev->is_virtfn)
+		return;
+
 	if (!pdev->pasid_enabled)
 		return;
 
@@ -436,12 +473,12 @@ EXPORT_SYMBOL_GPL(pci_restore_pasid_state);
 int pci_pasid_features(struct pci_dev *pdev)
 {
 	u16 supported;
+	struct pci_dev *pf = pci_physfn(pdev);
 
-	if (!pdev->pasid_cap)
+	if (!pf->pasid_cap)
 		return -EINVAL;
 
-	pci_read_config_word(pdev, pdev->pasid_cap + PCI_PASID_CAP,
-			     &supported);
+	pci_read_config_word(pf, pf->pasid_cap + PCI_PASID_CAP, &supported);
 
 	supported &= PCI_PASID_CAP_EXEC | PCI_PASID_CAP_PRIV;
 
@@ -492,12 +529,12 @@ EXPORT_SYMBOL_GPL(pci_prg_resp_pasid_required);
 int pci_max_pasids(struct pci_dev *pdev)
 {
 	u16 supported;
+	struct pci_dev *pf = pci_physfn(pdev);
 
-	if (!pdev->pasid_cap)
+	if (!pf->pasid_cap)
 		return -EINVAL;
 
-	pci_read_config_word(pdev, pdev->pasid_cap + PCI_PASID_CAP,
-			     &supported);
+	pci_read_config_word(pf, pf->pasid_cap + PCI_PASID_CAP, &supported);
 
 	supported = (supported & PASID_NUMBER_MASK) >> PASID_NUMBER_SHIFT;
 
-- 
2.21.0

[PATCH v7 6/7] PCI/ATS: Disable PF/VF ATS service independently

[sathyanarayanan.kuppuswamy]

From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

Currently all VF's needs to be disable their ATS service before
disabling the ATS service in corresponding PF device. But this logic is
incorrect and does not align with the spec. Also it might lead to
some power and performance impact in the system. As per PCIe spec r4.0,
sec 9.3.7.8, ATS Capabilities in VFs and their associated PFs may be
enabled/disabled independently. So remove this dependency logic in
enable/disable code.

Cc: Ashok Raj <ashok.raj@intel.com>
Cc: Keith Busch <keith.busch@intel.com>
Suggested-by: Ashok Raj <ashok.raj@intel.com>
Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 drivers/pci/ats.c   | 11 -----------
 include/linux/pci.h |  1 -
 2 files changed, 12 deletions(-)

diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
index 893674520bbf..e7db42128176 100644
--- a/drivers/pci/ats.c
+++ b/drivers/pci/ats.c
@@ -108,8 +108,6 @@ int pci_enable_ats(struct pci_dev *dev, int ps)
 		pdev = pci_physfn(dev);
 		if (pdev->ats_stu != ps)
 			return -EINVAL;
-
-		atomic_inc(&pdev->ats_ref_cnt);  /* count enabled VFs */
 	} else {
 		dev->ats_stu = ps;
 		ctrl |= PCI_ATS_CTRL_STU(dev->ats_stu - PCI_ATS_MIN_STU);
@@ -127,20 +125,11 @@ EXPORT_SYMBOL_GPL(pci_enable_ats);
  */
 void pci_disable_ats(struct pci_dev *dev)
 {
-	struct pci_dev *pdev;
 	u16 ctrl;
 
 	if (WARN_ON(!dev->ats_enabled))
 		return;
 
-	if (atomic_read(&dev->ats_ref_cnt))
-		return;		/* VFs still enabled */
-
-	if (dev->is_virtfn) {
-		pdev = pci_physfn(dev);
-		atomic_dec(&pdev->ats_ref_cnt);
-	}
-
 	pci_read_config_word(dev, dev->ats_cap + PCI_ATS_CTRL, &ctrl);
 	ctrl &= ~PCI_ATS_CTRL_ENABLE;
 	pci_write_config_word(dev, dev->ats_cap + PCI_ATS_CTRL, ctrl);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 782bca667010..2428414b2b9c 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -452,7 +452,6 @@ struct pci_dev {
 	};
 	u16		ats_cap;	/* ATS Capability offset */
 	u8		ats_stu;	/* ATS Smallest Translation Unit */
-	atomic_t	ats_ref_cnt;	/* Number of VFs with ATS enabled */
 #endif
 #ifdef CONFIG_PCI_PRI
 	u16		pri_cap;	/* PRI Capability offset */
-- 
2.21.0

[PATCH v7 7/7] PCI: Skip Enhanced Allocation (EA) initialization for VF device

[sathyanarayanan.kuppuswamy]

From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

As per PCIe r4.0, sec 9.3.6, VF must not implement Enhanced Allocation
Capability. So skip pci_ea_init() for virtual devices.

Cc: Ashok Raj <ashok.raj@intel.com>
Cc: Keith Busch <keith.busch@intel.com>
Suggested-by: Ashok Raj <ashok.raj@intel.com>
Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 drivers/pci/pci.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 1b27b5af3d55..266600a11769 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3025,6 +3025,13 @@ void pci_ea_init(struct pci_dev *dev)
 	int offset;
 	int i;
 
+	/*
+	 * Per PCIe r4.0, sec 9.3.6, VF must not implement Enhanced
+	 * Allocation Capability.
+	 */
+	if (dev->is_virtfn)
+		return;
+
 	/* find PCI EA capability in list */
 	ea = pci_find_capability(dev, PCI_CAP_ID_EA);
 	if (!ea)
-- 
2.21.0

Re: [PATCH v7 7/7] PCI: Skip Enhanced Allocation (EA) initialization for VF device

[Kuppuswamy Sathyanarayanan]

Hi Bjorn,

On 8/28/19 3:14 PM, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>
> As per PCIe r4.0, sec 9.3.6, VF must not implement Enhanced Allocation
> Capability. So skip pci_ea_init() for virtual devices.
>
> Cc: Ashok Raj <ashok.raj@intel.com>
> Cc: Keith Busch <keith.busch@intel.com>
> Suggested-by: Ashok Raj <ashok.raj@intel.com>
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
This patch was also dropped in your v8. Is this also intentional?
> ---
>   drivers/pci/pci.c | 7 +++++++
>   1 file changed, 7 insertions(+)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 1b27b5af3d55..266600a11769 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -3025,6 +3025,13 @@ void pci_ea_init(struct pci_dev *dev)
>   	int offset;
>   	int i;
>   
> +	/*
> +	 * Per PCIe r4.0, sec 9.3.6, VF must not implement Enhanced
> +	 * Allocation Capability.
> +	 */
> +	if (dev->is_virtfn)
> +		return;
> +
>   	/* find PCI EA capability in list */
>   	ea = pci_find_capability(dev, PCI_CAP_ID_EA);
>   	if (!ea)

-- 
Sathyanarayanan Kuppuswamy
Linux kernel developer

Re: [PATCH v7 7/7] PCI: Skip Enhanced Allocation (EA) initialization for VF device

[Bjorn Helgaas]

On Thu, Oct 03, 2019 at 10:21:24AM -0700, Kuppuswamy Sathyanarayanan wrote:
> Hi Bjorn,
> 
> On 8/28/19 3:14 PM, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
> > From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> > 
> > As per PCIe r4.0, sec 9.3.6, VF must not implement Enhanced Allocation
> > Capability. So skip pci_ea_init() for virtual devices.
> > 
> > Cc: Ashok Raj <ashok.raj@intel.com>
> > Cc: Keith Busch <keith.busch@intel.com>
> > Suggested-by: Ashok Raj <ashok.raj@intel.com>
> > Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> This patch was also dropped in your v8. Is this also intentional?

Yes, I dropped it because I didn't think there was much motivation for
it.

If a device is broken, i.e., a VF has an EA capability, this patch
silently returns.  The existing code would try to use the EA
capability and something would probably blow up, so in that sense,
this patch makes the hardware issue less visible.

If a device is correct, i.e., a VF does *not* have an EA capability,
pci_find_capability() will fail anyway, so this patch doesn't change
the functional behavior.

This patch *does* avoid the pci_find_capability() in that case, which
is a performance optimization.  We could merge it on that basis, but
we should try to quantify the benefit to see if it's really worthwhile
and the commit log should use that as the explicit motivation.

> > ---
> >   drivers/pci/pci.c | 7 +++++++
> >   1 file changed, 7 insertions(+)
> > 
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index 1b27b5af3d55..266600a11769 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -3025,6 +3025,13 @@ void pci_ea_init(struct pci_dev *dev)
> >   	int offset;
> >   	int i;
> > +	/*
> > +	 * Per PCIe r4.0, sec 9.3.6, VF must not implement Enhanced
> > +	 * Allocation Capability.
> > +	 */
> > +	if (dev->is_virtfn)
> > +		return;
> > +
> >   	/* find PCI EA capability in list */
> >   	ea = pci_find_capability(dev, PCI_CAP_ID_EA);
> >   	if (!ea)
> 
> -- 
> Sathyanarayanan Kuppuswamy
> Linux kernel developer
> 

Re: [PATCH v7 7/7] PCI: Skip Enhanced Allocation (EA) initialization for VF device

[Raj, Ashok]

On Thu, Oct 03, 2019 at 01:57:47PM -0500, Bjorn Helgaas wrote:
> On Thu, Oct 03, 2019 at 10:21:24AM -0700, Kuppuswamy Sathyanarayanan wrote:
> > Hi Bjorn,
> > 
> > On 8/28/19 3:14 PM, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
> > > From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> > > 
> > > As per PCIe r4.0, sec 9.3.6, VF must not implement Enhanced Allocation
> > > Capability. So skip pci_ea_init() for virtual devices.
> > > 
> > > Cc: Ashok Raj <ashok.raj@intel.com>
> > > Cc: Keith Busch <keith.busch@intel.com>
> > > Suggested-by: Ashok Raj <ashok.raj@intel.com>
> > > Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> > This patch was also dropped in your v8. Is this also intentional?
> 
> Yes, I dropped it because I didn't think there was much motivation for
> it.

Agreed!

> 
> If a device is broken, i.e., a VF has an EA capability, this patch
> silently returns.  The existing code would try to use the EA
> capability and something would probably blow up, so in that sense,
> this patch makes the hardware issue less visible.
> 
> If a device is correct, i.e., a VF does *not* have an EA capability,
> pci_find_capability() will fail anyway, so this patch doesn't change
> the functional behavior.


But do you think while at this can we atleast do a warning
to make sure HW probably messed up just after the EA capability
is read? Atleast it would be an early warning vs. it trying to break
later. Like the other issues we ran into with the PIN interrupt
accidently set in some hardware for VF's for instance. 

> 
> This patch *does* avoid the pci_find_capability() in that case, which
> is a performance optimization.  We could merge it on that basis, but
> we should try to quantify the benefit to see if it's really worthwhile
> and the commit log should use that as the explicit motivation.
> 
> > > ---
> > >   drivers/pci/pci.c | 7 +++++++
> > >   1 file changed, 7 insertions(+)
> > > 
> > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > index 1b27b5af3d55..266600a11769 100644
> > > --- a/drivers/pci/pci.c
> > > +++ b/drivers/pci/pci.c
> > > @@ -3025,6 +3025,13 @@ void pci_ea_init(struct pci_dev *dev)
> > >   	int offset;
> > >   	int i;
> > > +	/*
> > > +	 * Per PCIe r4.0, sec 9.3.6, VF must not implement Enhanced
> > > +	 * Allocation Capability.
> > > +	 */
> > > +	if (dev->is_virtfn)
> > > +		return;
> > > +
> > >   	/* find PCI EA capability in list */
> > >   	ea = pci_find_capability(dev, PCI_CAP_ID_EA);
> > >   	if (!ea)
> > 
> > -- 
> > Sathyanarayanan Kuppuswamy
> > Linux kernel developer
> > 

Re: [PATCH v7 0/7] Fix PF/VF dependency issue

[Bjorn Helgaas]

On Wed, Aug 28, 2019 at 03:14:00PM -0700, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> 
> Current implementation of ATS, PASID, PRI does not handle VF dependencies
> correctly. Following patches addresses this issue.
> 
> Changes since v6:
>  * Removed locking interfaces used in v6.
>  * Made necessary changes to PRI/PASID enable/disable calls to allow
>    register changes only for PF.
> 
> Changes since v5:
>  * Created new patches for PRI/PASID capability caching.
>  * Removed individual locks (pri_lock, pasid_lock) and added common
>    resource lock to synchronize PRI/PASID updates between PF/VF.
>  * Addressed comments from Bjorn Helgaas.
> 
> Changes since v4:
>  * Defined empty functions for pci_pri_init() and pci_pasid_init() for cases
>    where CONFIG_PCI_PRI and CONFIG_PCI_PASID are not enabled.
> 
> Changes since v3:
>  * Fixed critical path (lock context) in pci_restore_*_state functions.
> 
> Changes since v2:
>  * Added locking mechanism to synchronize accessing PF registers in VF.
>  * Removed spec compliance checks in patches.
>  * Addressed comments from Bjorn Helgaas.
> 
> Changes since v1:
>  * Added more details about the patches in commit log.
>  * Removed bulk spec check patch.
>  * Addressed comments from Bjorn Helgaas.
> 
> Kuppuswamy Sathyanarayanan (7):
>   PCI/ATS: Fix pci_prg_resp_pasid_required() dependency issues
>   PCI/ATS: Cache PRI capability check result
>   PCI/ATS: Cache PASID capability check result
>   PCI/ATS: Add PRI support for PCIe VF devices
>   PCI/ATS: Add PASID support for PCIe VF devices
>   PCI/ATS: Disable PF/VF ATS service independently
>   PCI: Skip Enhanced Allocation (EA) initialization for VF device

To make it easier to backport things, I think these should be
reordered so the important fixes are first, e.g., like this:

    PCI/ATS: Add PRI support for PCIe VF devices
    PCI/ATS: Add PASID support for PCIe VF devices
    PCI/ATS: Disable PF/VF ATS service independently
    PCI/ATS: Cache PRI capability check result
    PCI/ATS: Cache PASID capability check result

I don't think the following ones are actually needed:

    PCI: Skip Enhanced Allocation (EA) initialization for VF device
    PCI/ATS: Fix pci_prg_resp_pasid_required() dependency issues

I'll post a v8 with my proposal.

>  drivers/pci/ats.c       | 202 ++++++++++++++++++++++++++++------------
>  drivers/pci/pci.c       |   7 ++
>  include/linux/pci-ats.h |  12 ++-
>  include/linux/pci.h     |   3 +-
>  4 files changed, 159 insertions(+), 65 deletions(-)
> 
> -- 
> 2.21.0
>