Re: [PATCH v2 1/5] PCI/ATS: Add PRI support for PCIe VF devices

[Raj, Ashok]

On Wed, May 29, 2019 at 05:57:14PM -0500, Bjorn Helgaas wrote:
> On Mon, May 06, 2019 at 10:20:03AM -0700, sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx wrote:
> > From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx>
> > 
> > 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@xxxxxxxxx>
> > Cc: Keith Busch <keith.busch@xxxxxxxxx>
> > Suggested-by: Ashok Raj <ashok.raj@xxxxxxxxx>
> > Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx>
> > ---
> >  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;

This would be incorrect. Since if we never did any bind_mm to the PF
PRI would not have been enabled. Currently this is done in the IOMMU 
driver, and not in the device driver. 

I suppose we should enable PF capability if its not enabled. Same
comment would be applicable for PASID as well.


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