RE: [PATCH v1 3/7] fpga: dfl: Allow for ports with no local bar space.

["Zhang, Tianfei" <tianfei.zhang@intel.com>]

> -----Original Message-----
> From: Tom Rix <trix@redhat.com>
> Sent: Tuesday, February 15, 2022 11:06 PM
> To: Zhang, Tianfei <tianfei.zhang@intel.com>; Wu, Hao <hao.wu@intel.com>;
> mdf@kernel.org; Xu, Yilun <yilun.xu@intel.com>; linux-fpga@vger.kernel.org;
> linux-doc@vger.kernel.org; linux-kernel@vger.kernel.org
> Cc: corbet@lwn.net; Matthew Gerlach <matthew.gerlach@linux.intel.com>
> Subject: Re: [PATCH v1 3/7] fpga: dfl: Allow for ports with no local bar space.
> 
> 
> On 2/14/22 3:26 AM, Tianfei zhang wrote:
> > From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> >
> >  From a fpga partial reconfiguration standpoint, a port may not be
> > connected any local BAR space.  The port could be connected to a
> > different PCIe Physical Function (PF) or Virtual Function (VF), in
> > which case another driver instance would manage the endpoint.
> 
> It is not clear if this is part of iofs or a bug fix.

This is the new implementation/feature of IOFS.
On IOFS support multiple methods to access the AFU.
1. Legacy Model. This is used for N3000 and N5000 card.
In this model the entire AFU region is a unit of PR, and there is a Port device connected to this AFU.
On DFL perspective, there is "Next AFU" point to the AFU, and the "BarID" is  the PCIe Bar ID of AFU.
In this model, we can use the AFU APIs to access the entire AFU resource, like MMIO.
2. Micro-Personas in AFU. 
IOFS intruding new model for PR and AFU access.
Micro-Personas allow the RTL developer to designate their own AFU-defined PR regions. 
In this model the unit of PR is not the entire AFU, instead
the unit of PR can be any size block or blocks inside the AFU.
3. Multiple VFs per PR slot.
In this method, we can instance multiple VFs over SRIOV for one PR slot, and access the AFU resource
by different VFs in virtualization usage. In this case, the Port device would not connected to AFU (the BarID of Port device
should be set to invalid), so this patch want to support this use model.

> 
> > Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> > Signed-off-by: Tianfei Zhang <tianfei.zhang@intel.com>
> > ---
> >   drivers/fpga/dfl-pci.c | 8 ++++++++
> >   1 file changed, 8 insertions(+)
> >
> > diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c index
> > 4d68719e608f..8abd9b408403 100644
> > --- a/drivers/fpga/dfl-pci.c
> > +++ b/drivers/fpga/dfl-pci.c
> > @@ -243,6 +243,7 @@ static int find_dfls_by_default(struct pci_dev *pcidev,
> >   		v = readq(base + FME_HDR_CAP);
> >   		port_num = FIELD_GET(FME_CAP_NUM_PORTS, v);
> >
> > +		dev_info(&pcidev->dev, "port_num = %d\n", port_num);
> >   		WARN_ON(port_num > MAX_DFL_FPGA_PORT_NUM);
> >
> >   		for (i = 0; i < port_num; i++) {
> > @@ -258,6 +259,13 @@ static int find_dfls_by_default(struct pci_dev *pcidev,
> >   			 */
> >   			bar = FIELD_GET(FME_PORT_OFST_BAR_ID, v);
> >   			offset = FIELD_GET(FME_PORT_OFST_DFH_OFST, v);
> > +			if (bar >= PCI_STD_NUM_BARS) {
> 
> Is bar set to a better magic number that pci_std_num_bars ? maybe 0xff's
> 
> How do you tell between this case and broken hw ?

Yes, I agree that magic number is better, Currently the RTL using PCI_STD_NUM_BARS for an invalid PCIe bar number.

> 
> Move up a line and skip getting an offset that will not be used.

Yes, this line is not necessary, I will remove it on next version patch.

> 
> > +				dev_info(&pcidev->dev, "skipping port without
> local BAR space %d\n",
> > +					 bar);
> > +				continue;
> > +			} else {
> > +				dev_info(&pcidev->dev, "BAR %d offset %u\n",
> bar, offset);
> > +			}
> >   			start = pci_resource_start(pcidev, bar) + offset;
> >   			len = pci_resource_len(pcidev, bar) - offset;
> >
> 
> Is similar logic needed for else-if (port) block below this ?

I think, the else-if is not necessary. I will remove it on next version patch.
> 
> Tom