Subject: Re: [PATCH V5 1/6] powerpc/powernv: don't enable SRIOV when VF BAR has non 64bit-prefetchable BAR

From: Benjamin Herrenschmidt <[email-masked]>
Date: Mon, 12 Oct 2015 12:25:24 +0530

On Mon, 2015-10-12 at 10:58 +0800, Wei Yang wrote:
> On Fri, Oct 09, 2015 at 07:15:19PM +1100, Benjamin Herrenschmidt wrote:
> > On Fri, 2015-10-09 at 10:46 +0800, Wei Yang wrote:
> > > On PHB_IODA2, we enable SRIOV devices by mapping IOV BAR with M64 BARs. If
> > > a SRIOV device's IOV BAR is not 64bit-prefetchable, this is not assigned
> > > from 64bit prefetchable window, which means M64 BAR can't work on it.
> > 
> > Won't this cause a lot of devices to become unsupported for us ? Or do
> > all devices we care about have their BARs marked prefetchable ?
> > 
> 
> You are right. After doing so, some of the devices will not be supported.
> 
> Hmm, I thought you know this, since this strategy, use M64 BAR to map IOV
> BAR, is proposed by you. This patch doesn't change the function, while make it
> more explicit.

Right but we can make things better by also effectively allowing non
-prefetch BARs to sit in the prefetchable window of a bridge.

> > > The reason is PCI bridges support only 2 windows and the kernel code
> > > programs bridges in the way that one window is 32bit-nonprefetchable and
> > > the other one is 64bit-prefetchable. So if devices' IOV BAR is 64bit and
> > > non-prefetchable, it will be mapped into 32bit space and therefore M64
> > > cannot be used for it.
> > > 
> > > This patch makes this explicit.
> > 
> > So PCIe allows for non-prefetchable BARs to be put under prefetchable
> > bridge windows as long as the mapping done by the CPU doesn't prefetch,
> > I believe. Well it's a natural conclusion of the weird note "Additional
> > Guidance on the Prefetchable Bit in Memory Space BARs" page 596 of PCIe
> > spec v3.0... it also says that devices should be pretty much free to
> > set their prefetchable bit even if they have side effects so.
> > 
> > So maybe we should have that option, rather than just not using the
> > devices, allow them to be allocate via the prefetchable window...
> > 
> 
> Based on current linux kernel pci core resource size/assignment algorithm,
> when we have 64bit-prefetchable root window, only 64bit-prefetchable BAR could
> be assigned in this region. Refer to commit 5b28541552ef "PCI: Restrict 64-bit
> prefetchable bridge windows to 64-bit resources" for more detail.

The above patch was about avoiding 32-bit prefetchable, it's not about
64-bit non-prefetchable.

> So we have two facts here:
> 1. We want to use M64 BAR to map VFs, so that VF could be put into individual
> PE. And the M64 BAR just map 64bit window.
> 2. Current MMIO size/assignment algorithm in linux pci core only put
> 64bit-prefetchable BAR into 64bit prefetchable bridge window, no other BARs.
> 
> Your suggestion here is to put a non-prefetchable BAR in prefetchable bridge
> window? This would make the MMIO size/assignment algorithm more confusing.

Right, in the long run, both us and x86 might need to be able to do
that, though exclusively for PCIe. PCI/PCI-X bridges can prefetch.

Cheers,
Ben.

> > > Signed-off-by: Wei Yang <[email-masked]>
> > > Reviewed-by: Gavin Shan <[email-masked]>
> > > Acked-by: Alexey Kardashevskiy <[email-masked]>
> > > ---
> > >  arch/powerpc/platforms/powernv/pci-ioda.c | 25 +++++++++----------------
> > >  1 file changed, 9 insertions(+), 16 deletions(-)
> > > 
> > > diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
> > > index 85cbc96..8c031b5 100644
> > > --- a/arch/powerpc/platforms/powernv/pci-ioda.c
> > > +++ b/arch/powerpc/platforms/powernv/pci-ioda.c
> > > @@ -908,9 +908,6 @@ static int pnv_pci_vf_resource_shift(struct pci_dev *dev, int offset)
> > >  > > > > 	> > > > > > > 	> > > > if (!res->flags || !res->parent)
> > >  > > > > 	> > > > > > > 	> > > > > > > 	> > > > continue;
> > >  
> > > -> > > > 	> > > > > > > 	> > > > if (!pnv_pci_is_mem_pref_64(res->flags))
> > > -> > > > 	> > > > > > > 	> > > > > > > 	> > > > continue;
> > > -
> > >  > > > > 	> > > > > > > 	> > > > /*
> > >  > > > > 	> > > > > > > 	> > > >  * The actual IOV BAR range is determined by the start address
> > >  > > > > 	> > > > > > > 	> > > >  * and the actual size for num_vfs VFs BAR.  This check is to
> > > @@ -939,9 +936,6 @@ static int pnv_pci_vf_resource_shift(struct pci_dev *dev, int offset)
> > >  > > > > 	> > > > > > > 	> > > > if (!res->flags || !res->parent)
> > >  > > > > 	> > > > > > > 	> > > > > > > 	> > > > continue;
> > >  
> > > -> > > > 	> > > > > > > 	> > > > if (!pnv_pci_is_mem_pref_64(res->flags))
> > > -> > > > 	> > > > > > > 	> > > > > > > 	> > > > continue;
> > > -
> > >  > > > > 	> > > > > > > 	> > > > size = pci_iov_resource_size(dev, i + PCI_IOV_RESOURCES);
> > >  > > > > 	> > > > > > > 	> > > > res2 = *res;
> > >  > > > > 	> > > > > > > 	> > > > res->start += size * offset;
> > > @@ -1221,9 +1215,6 @@ static int pnv_pci_vf_assign_m64(struct pci_dev *pdev, u16 num_vfs)
> > >  > > > > 	> > > > > > > 	> > > > if (!res->flags || !res->parent)
> > >  > > > > 	> > > > > > > 	> > > > > > > 	> > > > continue;
> > >  
> > > -> > > > 	> > > > > > > 	> > > > if (!pnv_pci_is_mem_pref_64(res->flags))
> > > -> > > > 	> > > > > > > 	> > > > > > > 	> > > > continue;
> > > -
> > >  > > > > 	> > > > > > > 	> > > > for (j = 0; j < vf_groups; j++) {
> > >  > > > > 	> > > > > > > 	> > > > > > > 	> > > > do {
> > >  > > > > 	> > > > > > > 	> > > > > > > 	> > > > > > > 	> > > > win = find_next_zero_bit(&phb->ioda.m64_bar_alloc,
> > > @@ -1510,6 +1501,12 @@ int pnv_pci_sriov_enable(struct pci_dev *pdev, u16 num_vfs)
> > >  > > > > 	> > > > pdn = pci_get_pdn(pdev);
> > >  
> > >  > > > > 	> > > > if (phb->type == PNV_PHB_IODA2) {
> > > +> > > > 	> > > > > > > 	> > > > if (!pdn->vfs_expanded) {
> > > +> > > > 	> > > > > > > 	> > > > > > > 	> > > > dev_info(&pdev->dev, "don't support this SRIOV device"
> > > +> > > > 	> > > > > > > 	> > > > > > > 	> > > > > > > 	> > > > " with non 64bit-prefetchable IOV BAR\n");
> > > +> > > > 	> > > > > > > 	> > > > > > > 	> > > > return -ENOSPC;
> > > +> > > > 	> > > > > > > 	> > > > }
> > > +
> > >  > > > > 	> > > > > > > 	> > > > /* Calculate available PE for required VFs */
> > >  > > > > 	> > > > > > > 	> > > > mutex_lock(&phb->ioda.pe_alloc_mutex);
> > >  > > > > 	> > > > > > > 	> > > > pdn->offset = bitmap_find_next_zero_area(
> > > @@ -2775,9 +2772,10 @@ static void pnv_pci_ioda_fixup_iov_resources(struct pci_dev *pdev)
> > >  > > > > 	> > > > > > > 	> > > > if (!res->flags || res->parent)
> > >  > > > > 	> > > > > > > 	> > > > > > > 	> > > > continue;
> > >  > > > > 	> > > > > > > 	> > > > if (!pnv_pci_is_mem_pref_64(res->flags)) {
> > > -> > > > 	> > > > > > > 	> > > > > > > 	> > > > dev_warn(&pdev->dev, " non M64 VF BAR%d: %pR\n",
> > > +> > > > 	> > > > > > > 	> > > > > > > 	> > > > dev_warn(&pdev->dev, "Don't support SR-IOV with"
> > > +> > > > 	> > > > > > > 	> > > > > > > 	> > > > > > > 	> > > > > > > 	> > > > " non M64 VF BAR%d: %pR. \n",
> > >  > > > > 	> > > > > > > 	> > > > > > > 	> > > > > > > 	> > > >  i, res);
> > > -> > > > 	> > > > > > > 	> > > > > > > 	> > > > continue;
> > > +> > > > 	> > > > > > > 	> > > > > > > 	> > > > return;
> > >  > > > > 	> > > > > > > 	> > > > }
> > >  
> > >  > > > > 	> > > > > > > 	> > > > size = pci_iov_resource_size(pdev, i + PCI_IOV_RESOURCES);
> > > @@ -2796,11 +2794,6 @@ static void pnv_pci_ioda_fixup_iov_resources(struct pci_dev *pdev)
> > >  > > > > 	> > > > > > > 	> > > > res = &pdev->resource[i + PCI_IOV_RESOURCES];
> > >  > > > > 	> > > > > > > 	> > > > if (!res->flags || res->parent)
> > >  > > > > 	> > > > > > > 	> > > > > > > 	> > > > continue;
> > > -> > > > 	> > > > > > > 	> > > > if (!pnv_pci_is_mem_pref_64(res->flags)) {
> > > -> > > > 	> > > > > > > 	> > > > > > > 	> > > > dev_warn(&pdev->dev, "Skipping expanding VF BAR%d: %pR\n",
> > > -> > > > 	> > > > > > > 	> > > > > > > 	> > > > > > > 	> > > >  i, res);
> > > -> > > > 	> > > > > > > 	> > > > > > > 	> > > > continue;
> > > -> > > > 	> > > > > > > 	> > > > }
> > >  
> > >  > > > > 	> > > > > > > 	> > > > dev_dbg(&pdev->dev, " Fixing VF BAR%d: %pR to\n", i, res);
> > >  > > > > 	> > > > > > > 	> > > > size = pci_iov_resource_size(pdev, i + PCI_IOV_RESOURCES);
>