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@xxxxxxxxxxxxxxx wrote:
> > > > > > > From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx>
> > > > > > > 
> > > > > > > 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