Re: [PATCH V11 01/17] PCI/IOV: Export interface for retrieve VF's BDF

[Bjorn Helgaas <bhelgaas@google.com>]

On Thu, Jan 15, 2015 at 10:27:51AM +0800, Wei Yang wrote:
> When implementing the SR-IOV on PowerNV platform, some resource reservation
> is needed for VFs which don't exist at the bootup stage. To do the match
> between resources and VFs, the code need to get the VF's BDF in advance.
> 
> In this patch, it exports the interface to retrieve VF's BDF:
>    * Make the virtfn_bus as an interface
>    * Make the virtfn_devfn as an interface
>    * Rename them with more specific name
>    * Code cleanup in pci_sriov_resource_alignment()

You use these in this path:

    pci_enable_sriov
      sriov_enable
	pcibios_sriov_enable
	  add_dev_pci_info
	    for (i = 0; i < pci_sriov_get_totalvfs(pdev))
	      add_one_dev_pci_info(..., pci_iov_virtfn_bus(), ...)  <---
		pdn = kzalloc
		pdn->busno = busno
		pdn->devfn = devfn
		list_add_tail(&pdn->list, &parent->child_list)

It looks like this sets up a struct pci_dn for each VF.

Could the struct pci_dn setup be done in pcibios_add_device() instead?
Then each VF we enumerate would set up its own struct pci_dn.  That would
be a lot nicer than using this hook to iterate over all possible VFs.

You also use them in some PE setup in a similar path:

    pci_enable_sriov
      sriov_enable
        pcibios_sriov_enable
          pnv_pci_sriov_enable
            pnv_pci_vf_assign_m64
            pnv_pci_vf_resource_shift
            pnv_ioda_setup_vf_PE
              for (i = 0; i < vf_num; i++)
                pe->rid = pci_iov_virtfn_bus(...)  <---
                pnv_ioda_configure_pe(phb, pe)
                pe->tce32_table = kzalloc
                pnv_pci_ioda2_setup_dma_pe

Could this PE setup also be done in pcibios_device() when the VF device
itself is enumerated?  I think that would be a nicer design if it's
possible.

I'd prefer to avoid exporting pci_iov_virtfn_bus() and
pci_iov_virtfn_devfn() if possible, because they're only safe to call when
VFs are enabled (because offset & stride depend on numVFs).

> Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com>
> ---
>  drivers/pci/iov.c   |   22 +++++++++++++---------
>  include/linux/pci.h |   11 +++++++++++
>  2 files changed, 24 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> index ea3a82c..e76d1a0 100644
> --- a/drivers/pci/iov.c
> +++ b/drivers/pci/iov.c
> @@ -19,14 +19,18 @@
>  
>  #define VIRTFN_ID_LEN	16
>  
> -static inline u8 virtfn_bus(struct pci_dev *dev, int id)
> +int pci_iov_virtfn_bus(struct pci_dev *dev, int id)
>  {
> +	if (!dev->is_physfn)
> +		return -EINVAL;
>  	return dev->bus->number + ((dev->devfn + dev->sriov->offset +
>  				    dev->sriov->stride * id) >> 8);
>  }
>  
> -static inline u8 virtfn_devfn(struct pci_dev *dev, int id)
> +int pci_iov_virtfn_devfn(struct pci_dev *dev, int id)
>  {
> +	if (!dev->is_physfn)
> +		return -EINVAL;
>  	return (dev->devfn + dev->sriov->offset +
>  		dev->sriov->stride * id) & 0xff;
>  }
> @@ -62,7 +66,7 @@ static inline void pci_iov_max_bus_range(struct pci_dev *dev)
>  
>  	for ( ; total >= 0; total--) {
>  		pci_iov_set_numvfs(dev, total);
> -		busnr = virtfn_bus(dev, iov->total_VFs - 1);
> +		busnr = pci_iov_virtfn_bus(dev, iov->total_VFs - 1);
>  		if (busnr > max)
>  			max = busnr;
>  	}
> @@ -108,7 +112,7 @@ static int virtfn_add(struct pci_dev *dev, int id, int reset)
>  	struct pci_bus *bus;
>  
>  	mutex_lock(&iov->dev->sriov->lock);
> -	bus = virtfn_add_bus(dev->bus, virtfn_bus(dev, id));
> +	bus = virtfn_add_bus(dev->bus, pci_iov_virtfn_bus(dev, id));
>  	if (!bus)
>  		goto failed;
>  
> @@ -116,7 +120,7 @@ static int virtfn_add(struct pci_dev *dev, int id, int reset)
>  	if (!virtfn)
>  		goto failed0;
>  
> -	virtfn->devfn = virtfn_devfn(dev, id);
> +	virtfn->devfn = pci_iov_virtfn_devfn(dev, id);
>  	virtfn->vendor = dev->vendor;
>  	pci_read_config_word(dev, iov->pos + PCI_SRIOV_VF_DID, &virtfn->device);
>  	pci_setup_device(virtfn);
> @@ -179,8 +183,8 @@ static void virtfn_remove(struct pci_dev *dev, int id, int reset)
>  	struct pci_sriov *iov = dev->sriov;
>  
>  	virtfn = pci_get_domain_bus_and_slot(pci_domain_nr(dev->bus),
> -					     virtfn_bus(dev, id),
> -					     virtfn_devfn(dev, id));
> +					     pci_iov_virtfn_bus(dev, id),
> +					     pci_iov_virtfn_devfn(dev, id));
>  	if (!virtfn)
>  		return;
>  
> @@ -255,7 +259,7 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
>  	iov->offset = offset;
>  	iov->stride = stride;
>  
> -	if (virtfn_bus(dev, nr_virtfn - 1) > dev->bus->busn_res.end) {
> +	if (pci_iov_virtfn_bus(dev, nr_virtfn - 1) > dev->bus->busn_res.end) {
>  		dev_err(&dev->dev, "SR-IOV: bus number out of range\n");
>  		return -ENOMEM;
>  	}
> @@ -551,7 +555,7 @@ resource_size_t pci_sriov_resource_alignment(struct pci_dev *dev, int resno)
>  	if (!reg)
>  		return 0;
>  
> -	 __pci_read_base(dev, pci_bar_unknown, &tmp, reg);
> +	__pci_read_base(dev, pci_bar_unknown, &tmp, reg);
>  	return resource_alignment(&tmp);
>  }
>  
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 360a966..74ef944 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1658,6 +1658,9 @@ int pci_ext_cfg_avail(void);
>  void __iomem *pci_ioremap_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_enable_sriov(struct pci_dev *dev, int nr_virtfn);
>  void pci_disable_sriov(struct pci_dev *dev);
>  int pci_num_vf(struct pci_dev *dev);
> @@ -1665,6 +1668,14 @@ int pci_vfs_assigned(struct pci_dev *dev);
>  int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs);
>  int pci_sriov_get_totalvfs(struct pci_dev *dev);
>  #else
> +static inline int pci_iov_virtfn_bus(struct pci_dev *dev, int id)
> +{
> +	return -ENOSYS;
> +}
> +static inline int pci_iov_virtfn_devfn(struct pci_dev *dev, int id)
> +{
> +	return -ENOSYS;
> +}
>  static inline int pci_enable_sriov(struct pci_dev *dev, int nr_virtfn)
>  { return -ENODEV; }
>  static inline void pci_disable_sriov(struct pci_dev *dev) { }
> -- 
> 1.7.9.5
>