LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Bjorn Helgaas <helgaas@kernel.org>
To: sathyanarayanan.kuppuswamy@linux.intel.com
Cc: linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org,
	ashok.raj@intel.com, keith.busch@intel.com
Subject: Re: [PATCH v2 1/5] PCI/ATS: Add PRI support for PCIe VF devices
Date: Wed, 29 May 2019 17:57:14 -0500	[thread overview]
Message-ID: <20190529225714.GE28250@google.com> (raw)
In-Reply-To: <f773440c0eee2a8d4e5d6e2856717404ac836458.1557162861.git.sathyanarayanan.kuppuswamy@linux.intel.com>

On Mon, May 06, 2019 at 10:20:03AM -0700, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> 
> When IOMMU tries to enable PRI for VF device in
> iommu_enable_dev_iotlb(), it always fails because PRI support for PCIe
> VF device is currently broken in PCIE driver. 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 Page Request Interface (PRI) of the PF and not implement it.
> Hence we need to create exception for handling the PRI support for PCIe
> VF device.
> 
> Since PRI is shared between PF/VF devices, following rules should apply.
> 
> 1. Enable PRI in VF only if its already enabled in PF.
> 2. When enabling/disabling PRI for VF, instead of configuring the
> registers just increase/decrease the usage count (pri_ref_cnt) of PF.
> 3. Disable PRI in PF only if pr_ref_cnt is zero.

s/pr_ref_cnt/pri_ref_cnt/

> 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   | 53 +++++++++++++++++++++++++++++++++++++++++++--
>  include/linux/pci.h |  1 +
>  2 files changed, 52 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
> index 97c08146534a..5582e5d83a3f 100644
> --- a/drivers/pci/ats.c
> +++ b/drivers/pci/ats.c
> @@ -181,12 +181,39 @@ int pci_enable_pri(struct pci_dev *pdev, u32 reqs)
>  	u16 control, status;
>  	u32 max_requests;
>  	int pos;
> +	struct pci_dev *pf;
>  
>  	if (WARN_ON(pdev->pri_enabled))
>  		return -EBUSY;
>  
>  	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_PRI);
> -	if (!pos)
> +
> +	if (pdev->is_virtfn) {
> +		/*
> +		 * Per PCIe r4.0, sec 9.3.7.11, VF must not implement PRI
> +		 * Capability.
> +		 */
> +		if (pos) {
> +			dev_err(&pdev->dev, "VF must not implement PRI");
> +			return -EINVAL;
> +		}

This seems gratuitous.  It finds implementation errors, but since we
correctly use the PF here anyway, it doesn't *need* to prevent PRI on
the VF from working.

I think you should just have:

  if (pdev->is_virtfn) {
    pf = pci_physfn(pdev);
    if (!pf->pri_enabled)
      return -EINVAL;

    pdev->pri_enabled = 1;
    atomic_inc(&pf->pri_ref_cnt);
  }

  pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_PRI);
  if (!pos)
    return -EINVAL;

> +		pf = pci_physfn(pdev);
> +
> +		/* If VF config does not match with PF, return error */
> +		if (!pf->pri_enabled)
> +			return -EINVAL;
> +
> +		pdev->pri_reqs_alloc = pf->pri_reqs_alloc;

Is there any point in setting vf->pri_reqs_alloc?  I don't think it's
used for anything since pri_reqs_alloc is only used to write the PF
capability, and we only do that for the PF.

> +		pdev->pri_enabled = 1;
> +
> +		/* Increment PF PRI refcount */

Superfluous comment, since that's exactly what the code says.  It's
always good when the code is so clear that it doesn't require comments :)

> +		atomic_inc(&pf->pri_ref_cnt);
> +
> +		return 0;
> +	}
> +
> +	if (pdev->is_physfn && !pos)
>  		return -EINVAL;
>  
>  	pci_read_config_word(pdev, pos + PCI_PRI_STATUS, &status);
> @@ -202,7 +229,6 @@ int pci_enable_pri(struct pci_dev *pdev, u32 reqs)
>  	pci_write_config_word(pdev, pos + PCI_PRI_CTRL, control);
>  
>  	pdev->pri_enabled = 1;
> -
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(pci_enable_pri);
> @@ -217,10 +243,27 @@ void pci_disable_pri(struct pci_dev *pdev)
>  {
>  	u16 control;
>  	int pos;
> +	struct pci_dev *pf;
>  
>  	if (WARN_ON(!pdev->pri_enabled))
>  		return;
>  
> +	/* All VFs should be disabled before disabling PF */
> +	if (atomic_read(&pdev->pri_ref_cnt))
> +		return;
> +
> +	if (pdev->is_virtfn) {
> +		/* Since VF shares PRI with PF, use PF config. */
> +		pf = pci_physfn(pdev);
> +
> +		/* Decrement PF PRI refcount */
> +		atomic_dec(&pf->pri_ref_cnt);
> +
> +		pdev->pri_enabled = 0;
> +
> +		return;
> +	}
> +
>  	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_PRI);
>  	if (!pos)
>  		return;
> @@ -246,6 +289,9 @@ void pci_restore_pri_state(struct pci_dev *pdev)
>  	if (!pdev->pri_enabled)
>  		return;
>  
> +	if (pdev->is_virtfn)
> +		return;
> +
>  	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_PRI);
>  	if (!pos)
>  		return;
> @@ -270,6 +316,9 @@ int pci_reset_pri(struct pci_dev *pdev)
>  	if (WARN_ON(pdev->pri_enabled))
>  		return -EBUSY;
>  
> +	if (pdev->is_virtfn)
> +		return 0;
> +
>  	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_PRI);
>  	if (!pos)
>  		return -EINVAL;
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 77448215ef5b..699c79c99a39 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -450,6 +450,7 @@ struct pci_dev {
>  #endif
>  #ifdef CONFIG_PCI_PRI
>  	u32		pri_reqs_alloc; /* Number of PRI requests allocated */
> +	atomic_t	pri_ref_cnt;	/* Number of VFs with PRI enabled */
>  #endif
>  #ifdef CONFIG_PCI_PASID
>  	u16		pasid_features;
> -- 
> 2.20.1
> 

  reply	other threads:[~2019-05-29 22:57 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-06 17:20 [PATCH v2 0/5] Fix PF/VF dependency issues sathyanarayanan.kuppuswamy
2019-05-06 17:20 ` [PATCH v2 1/5] PCI/ATS: Add PRI support for PCIe VF devices sathyanarayanan.kuppuswamy
2019-05-29 22:57   ` Bjorn Helgaas [this message]
2019-05-29 23:04     ` Raj, Ashok
2019-05-29 23:24       ` sathyanarayanan kuppuswamy
2019-05-30 13:20         ` Bjorn Helgaas
2019-05-30 13:17       ` Bjorn Helgaas
2019-05-30 17:20         ` Raj, Ashok
2019-05-30 17:39           ` sathyanarayanan kuppuswamy
2019-05-06 17:20 ` [PATCH v2 2/5] PCI/ATS: Add PASID " sathyanarayanan.kuppuswamy
2019-05-21 23:21   ` sathyanarayanan kuppuswamy
2019-05-29 23:03   ` Bjorn Helgaas
2019-05-06 17:20 ` [PATCH v2 3/5] PCI/ATS: Skip VF ATS initialization if PF does not implement it sathyanarayanan.kuppuswamy
2019-05-06 22:35   ` Elliott, Robert (Servers)
2019-05-30  2:53   ` Bjorn Helgaas
2019-05-06 17:20 ` [PATCH v2 4/5] PCI/ATS: Disable PF/VF ATS service independently sathyanarayanan.kuppuswamy
2019-05-06 17:20 ` [PATCH v2 5/5] PCI: Skip Enhanced Allocation (EA) initalization for VF device sathyanarayanan.kuppuswamy

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190529225714.GE28250@google.com \
    --to=helgaas@kernel.org \
    --cc=ashok.raj@intel.com \
    --cc=keith.busch@intel.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=sathyanarayanan.kuppuswamy@linux.intel.com \
    --subject='Re: [PATCH v2 1/5] PCI/ATS: Add PRI support for PCIe VF devices' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).