netdev - Re: [PATCH mlx5-next 1/7] PCI/IOV: Provide internal VF index

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Thu, 23 Sep 2021 09:35:32 +0300
From:   Leon Romanovsky <leon@...nel.org>
To:     Bjorn Helgaas <helgaas@...nel.org>
Cc:     Doug Ledford <dledford@...hat.com>,
        Jason Gunthorpe <jgg@...dia.com>,
        Alex Williamson <alex.williamson@...hat.com>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org,
        linux-rdma@...r.kernel.org, netdev@...r.kernel.org,
        Saeed Mahameed <saeedm@...dia.com>,
        Yishai Hadas <yishaih@...dia.com>
Subject: Re: [PATCH mlx5-next 1/7] PCI/IOV: Provide internal VF index

On Wed, Sep 22, 2021 at 04:59:30PM -0500, Bjorn Helgaas wrote:
> On Wed, Sep 22, 2021 at 01:38:50PM +0300, Leon Romanovsky wrote:
> > From: Jason Gunthorpe <jgg@...dia.com>
> > 
> > The PCI core uses the VF index internally, often called the vf_id,
> > during the setup of the VF, eg pci_iov_add_virtfn().
> > 
> > This index is needed for device drivers that implement live migration
> > for their internal operations that configure/control their VFs.
> >
> > Specifically, mlx5_vfio_pci driver that is introduced in coming patches
> > from this series needs it and not the bus/device/function which is
> > exposed today.
> > 
> > Add pci_iov_vf_id() which computes the vf_id by reversing the math that
> > was used to create the bus/device/function.
> > 
> > Signed-off-by: Yishai Hadas <yishaih@...dia.com>
> > Signed-off-by: Jason Gunthorpe <jgg@...dia.com>
> > Signed-off-by: Leon Romanovsky <leonro@...dia.com>
> 
> Acked-by: Bjorn Helgaas <bhelgaas@...gle.com>
> 
> mlx5_core_sriov_set_msix_vec_count() looks like it does basically the
> same thing as pci_iov_vf_id() by iterating through VFs until it finds
> one with a matching devfn (although it *doesn't* check for a matching
> bus number, which seems like a bug).
> 
> Maybe that should use pci_iov_vf_id()?

Yes, I gave same comment internally and we decided to simply reduce the
amount of changes in mlx5_core to have less distractions and submit as a
followup. Most likely will add this hunk in v1.

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/sriov.c b/drivers/net/ethernet/mellanox/mlx5/core/sriov.c
index e8185b69ac6c..b66be0b4244a 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/sriov.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/sriov.c
@@ -209,15 +209,8 @@ int mlx5_core_sriov_set_msix_vec_count(struct pci_dev *vf, int msix_vec_count)
        /* Reversed translation of PCI VF function number to the internal
         * function_id, which exists in the name of virtfn symlink.
         */
-       for (id = 0; id < pci_num_vf(pf); id++) {
-               if (!sriov->vfs_ctx[id].enabled)
-                       continue;
-
-               if (vf->devfn == pci_iov_virtfn_devfn(pf, id))
-                       break;
-       }
-
-       if (id == pci_num_vf(pf) || !sriov->vfs_ctx[id].enabled)
+       id = pci_iov_vf_id(vf);
+       if (id < 0 || !sriov->vfs_ctx[id].enabled)
                return -EINVAL;

        return mlx5_set_msix_vec_count(dev, id + 1, msix_vec_count);

Thanks

> 
> > ---
> >  drivers/pci/iov.c   | 14 ++++++++++++++
> >  include/linux/pci.h |  7 ++++++-
> >  2 files changed, 20 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> > index dafdc652fcd0..e7751fa3fe0b 100644
> > --- a/drivers/pci/iov.c
> > +++ b/drivers/pci/iov.c
> > @@ -33,6 +33,20 @@ int pci_iov_virtfn_devfn(struct pci_dev *dev, int vf_id)
> >  }
> >  EXPORT_SYMBOL_GPL(pci_iov_virtfn_devfn);
> >  
> > +int pci_iov_vf_id(struct pci_dev *dev)
> > +{
> > +	struct pci_dev *pf;
> > +
> > +	if (!dev->is_virtfn)
> > +		return -EINVAL;
> > +
> > +	pf = pci_physfn(dev);
> > +	return (((dev->bus->number << 8) + dev->devfn) -
> > +		((pf->bus->number << 8) + pf->devfn + pf->sriov->offset)) /
> > +	       pf->sriov->stride;
> > +}
> > +EXPORT_SYMBOL_GPL(pci_iov_vf_id);
> > +
> >  /*
> >   * Per SR-IOV spec sec 3.3.10 and 3.3.11, First VF Offset and VF Stride may
> >   * change when NumVFs changes.
> > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > index cd8aa6fce204..4d6c73506e18 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -2153,7 +2153,7 @@ void __iomem *pci_ioremap_wc_bar(struct pci_dev *pdev, int bar);
> >  #ifdef CONFIG_PCI_IOV
> >  int pci_iov_virtfn_bus(struct pci_dev *dev, int id);
> >  int pci_iov_virtfn_devfn(struct pci_dev *dev, int id);
> > -
> > +int pci_iov_vf_id(struct pci_dev *dev);
> >  int pci_enable_sriov(struct pci_dev *dev, int nr_virtfn);
> >  void pci_disable_sriov(struct pci_dev *dev);
> >  
> > @@ -2181,6 +2181,11 @@ static inline int pci_iov_virtfn_devfn(struct pci_dev *dev, int id)
> >  {
> >  	return -ENOSYS;
> >  }
> > +static inline int pci_iov_vf_id(struct pci_dev *dev)
> > +{
> > +	return -ENOSYS;
> > +}
> > +
> 
> Drop the blank line to match the surrounding stubs.

Sure, thanks

> 
> >  static inline int pci_enable_sriov(struct pci_dev *dev, int nr_virtfn)
> >  { return -ENODEV; }
>