diff mbox series

[2/2] fpga: dfl: look for vendor specific capability

Message ID 20201117012552.262149-3-matthew.gerlach@linux.intel.com (mailing list archive)
State Superseded, archived
Headers show
Series fpga: dfl: optional VSEC for start of dfl | expand

Commit Message

matthew.gerlach@linux.intel.com Nov. 17, 2020, 1:25 a.m. UTC
From: Matthew Gerlach <matthew.gerlach@linux.intel.com>

A DFL may not begin at offset 0 of BAR 0.  A PCIe vendor
specific capability can be used to specify the start of a
number of DFLs.

Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
---
 Documentation/fpga/dfl.rst | 10 +++++
 drivers/fpga/dfl-pci.c     | 88 +++++++++++++++++++++++++++++++++++++-
 2 files changed, 97 insertions(+), 1 deletion(-)

Comments

Xu Yilun Nov. 17, 2020, 7:56 a.m. UTC | #1
On Mon, Nov 16, 2020 at 05:25:52PM -0800, matthew.gerlach@linux.intel.com wrote:
> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> 
> A DFL may not begin at offset 0 of BAR 0.  A PCIe vendor
> specific capability can be used to specify the start of a
> number of DFLs.
> 
> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> ---
>  Documentation/fpga/dfl.rst | 10 +++++
>  drivers/fpga/dfl-pci.c     | 88 +++++++++++++++++++++++++++++++++++++-
>  2 files changed, 97 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/fpga/dfl.rst b/Documentation/fpga/dfl.rst
> index 0404fe6ffc74..c81ceb1e79e2 100644
> --- a/Documentation/fpga/dfl.rst
> +++ b/Documentation/fpga/dfl.rst
> @@ -501,6 +501,16 @@ Developer only needs to provide a sub feature driver with matched feature id.
>  FME Partial Reconfiguration Sub Feature driver (see drivers/fpga/dfl-fme-pr.c)
>  could be a reference.
>  
> +Location of DFLs on PCI bus
> +===========================
> +The start of the DFL is assumed to be offset 0 of bar 0.
> +Alternatively, a vendor specific capability structure can be used to
> +specify the location of one or more DFLs.  Intel has reserved the
> +vendor specific id of 0x43 for this purpose.  The vendor specific
> +data begins with a 4 byte count of the number of DFLs followed 4 byte
> +Offset/BIR fields for each DFL. Bits 2:0 of Offset/BIR field indicates
> +the BAR, and bits 31:3 form the 8 byte aligned offset where bits 2:0 are
> +zero.
>  
>  Open discussion
>  ===============
> diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c
> index b1b157b41942..5418e8bf2496 100644
> --- a/drivers/fpga/dfl-pci.c
> +++ b/drivers/fpga/dfl-pci.c
> @@ -27,6 +27,13 @@
>  #define DRV_VERSION	"0.8"
>  #define DRV_NAME	"dfl-pci"
>  
> +#define PCI_VNDR_ID_DFLS 0x43
> +
> +#define PCI_VNDR_DFLS_CNT_OFFSET 8
> +#define PCI_VNDR_DFLS_RES_OFFSET 0x0c
> +
> +#define PCI_VND_DFLS_RES_BAR_MASK 0x7

We could define the mask by GENMASK().

Also another macro PCI_VND_DFLS_RES_OFFSET_MASK is needed.

> +
>  struct cci_drvdata {
>  	struct dfl_fpga_cdev *cdev;	/* container device */
>  };
> @@ -119,6 +126,82 @@ static int *cci_pci_create_irq_table(struct pci_dev *pcidev, unsigned int nvec)
>  	return table;
>  }
>  
> +static int find_dfl_in_cfg(struct pci_dev *pcidev,
> +			   struct dfl_fpga_enum_info *info)
> +{
> +	u32 bar, offset, vndr_hdr, dfl_cnt, dfl_res;
> +	int dfl_res_off, i, voff = 0;
> +	resource_size_t start, len;
> +
> +	while ((voff = pci_find_next_ext_capability(pcidev, voff, PCI_EXT_CAP_ID_VNDR))) {
> +
> +		pci_read_config_dword(pcidev, voff + PCI_VNDR_HEADER, &vndr_hdr);
> +
> +		dev_dbg(&pcidev->dev,
> +			"vendor-specific capability id 0x%x, rev 0x%x len 0x%x\n",
> +			PCI_VNDR_HEADER_ID(vndr_hdr),
> +			PCI_VNDR_HEADER_REV(vndr_hdr),
> +			PCI_VNDR_HEADER_LEN(vndr_hdr));
> +
> +		if (PCI_VNDR_HEADER_ID(vndr_hdr) == PCI_VNDR_ID_DFLS)
> +			break;
> +	}
> +
> +	if (!voff) {
> +		dev_dbg(&pcidev->dev, "%s no VSEC found\n", __func__);
> +		return -ENODEV;
> +	}
> +
> +	pci_read_config_dword(pcidev, voff + PCI_VNDR_DFLS_CNT_OFFSET, &dfl_cnt);
> +	dev_info(&pcidev->dev, "dfl_cnt %d\n", dfl_cnt);

dev_dbg() is better?

> +	for (i = 0; i < dfl_cnt; i++) {
> +		dfl_res_off = voff + PCI_VNDR_DFLS_RES_OFFSET +
> +				      (i * sizeof(dfl_res));
> +		pci_read_config_dword(pcidev, dfl_res_off, &dfl_res);
> +
> +		dev_dbg(&pcidev->dev, "dfl_res 0x%x\n", dfl_res);
> +
> +		bar = dfl_res & PCI_VND_DFLS_RES_BAR_MASK;

FIELD_GET is better?

> +
> +		if (bar >= PCI_STD_NUM_BARS) {
> +			dev_err(&pcidev->dev, "%s bad bar number %d\n",
> +				__func__, bar);
> +			return -EINVAL;
> +		}
> +
> +		len = pci_resource_len(pcidev, bar);
> +
> +		if (len == 0) {
> +			dev_err(&pcidev->dev, "%s unmapped bar number %d\n",
> +				__func__, bar);
> +			return -EINVAL;
> +		}
> +
> +		offset = dfl_res & ~PCI_VND_DFLS_RES_BAR_MASK;

ditto

> +
> +		if (offset >= len) {
> +			dev_err(&pcidev->dev, "%s bad offset %u >= %llu\n",
> +				__func__, offset, len);
> +			return -EINVAL;
> +		}
> +
> +		dev_info(&pcidev->dev, "%s BAR %d offset 0x%x\n", __func__, bar, offset);

dev_dbg()?

> +
> +		start = pci_resource_start(pcidev, bar) + offset;
> +		len -= offset;

With these code, I have the following assumption:

1. There is only one DFL in one bar, multiple DFLs requires multiple
bars.

2. The DFL region is from the "offset" to the end of the bar.

Are they correct? If yes maybe we should specify them clearly in Doc.

> +
> +		if (!PAGE_ALIGNED(start)) {
> +			dev_err(&pcidev->dev, "%s unaliged start 0x%llx\n",
> +				__func__, start);
> +			return -EINVAL;
> +		}
> +
> +		dfl_fpga_enum_info_add_dfl(info, start, len);

Do we need some region overlapping check in this func? So we could find
the HW problem (e.g. same bar num for multiple DFLs) in early stage.

> +	}
> +
> +	return 0;
> +}
> +
>  static int find_dfl_in_bar0(struct pci_dev *pcidev,
>  			    struct dfl_fpga_enum_info *info)
>  {
> @@ -221,7 +304,10 @@ static int cci_enumerate_feature_devs(struct pci_dev *pcidev)
>  			goto irq_free_exit;
>  	}
>  
> -	ret = find_dfl_in_bar0(pcidev, info);
> +	ret = find_dfl_in_cfg(pcidev, info);
> +
> +	if (ret)
> +		ret = find_dfl_in_bar0(pcidev, info);

The patch is more than the relocation support for DFL. Actually it
introduced a different way of DFL finding.

Previously it starts at bar0 offset 0, find dfl fme first, then find
dfl port according to fme header registers. Now it enumerates every DFL
by PCIe VSEC.

Maybe we should add more description about the change and why.

Thanks,
Yilun

>  
>  	if (ret)
>  		goto irq_free_exit;
> -- 
> 2.25.2
Wu, Hao Nov. 17, 2020, 9:47 a.m. UTC | #2
> Subject: [PATCH 2/2] fpga: dfl: look for vendor specific capability
> 
> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> 
> A DFL may not begin at offset 0 of BAR 0.  A PCIe vendor
> specific capability can be used to specify the start of a
> number of DFLs.
> 
> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> ---
>  Documentation/fpga/dfl.rst | 10 +++++
>  drivers/fpga/dfl-pci.c     | 88 +++++++++++++++++++++++++++++++++++++-
>  2 files changed, 97 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/fpga/dfl.rst b/Documentation/fpga/dfl.rst
> index 0404fe6ffc74..c81ceb1e79e2 100644
> --- a/Documentation/fpga/dfl.rst
> +++ b/Documentation/fpga/dfl.rst
> @@ -501,6 +501,16 @@ Developer only needs to provide a sub feature
> driver with matched feature id.
>  FME Partial Reconfiguration Sub Feature driver (see drivers/fpga/dfl-fme-
> pr.c)
>  could be a reference.
> 
> +Location of DFLs on PCI bus

Location of DFLs on PCI Device 

> +===========================
> +The start of the DFL is assumed to be offset 0 of bar 0.

the first DFL

> +Alternatively, a vendor specific capability structure can be used to
> +specify the location of one or more DFLs.  Intel has reserved the

I believe this capability is used to specify all DFLs on this the PCI device.

> +vendor specific id of 0x43 for this purpose.  The vendor specific

VSEC ID is 0x43. 

One more question here is, does it require vendor id to be intel firstly?
Or other vendors could implement the same one but with a different id?

> +data begins with a 4 byte count of the number of DFLs followed 4 byte

vendor specific register 

> +Offset/BIR fields for each DFL. Bits 2:0 of Offset/BIR field indicates
> +the BAR, and bits 31:3 form the 8 byte aligned offset where bits 2:0 are
> +zero.
> 
>  Open discussion
>  ===============
> diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c
> index b1b157b41942..5418e8bf2496 100644
> --- a/drivers/fpga/dfl-pci.c
> +++ b/drivers/fpga/dfl-pci.c
> @@ -27,6 +27,13 @@
>  #define DRV_VERSION	"0.8"
>  #define DRV_NAME	"dfl-pci"
> 
> +#define PCI_VNDR_ID_DFLS 0x43

What about PCI_VSEC_ID_INTEL_DFLS?

Is it possible a different ID chosen by different vendor?

> +
> +#define PCI_VNDR_DFLS_CNT_OFFSET 8
> +#define PCI_VNDR_DFLS_RES_OFFSET 0x0c

What about VSEC_DFLS_CNT ?

> +
> +#define PCI_VND_DFLS_RES_BAR_MASK 0x7
> +
>  struct cci_drvdata {
>  	struct dfl_fpga_cdev *cdev;	/* container device */
>  };
> @@ -119,6 +126,82 @@ static int *cci_pci_create_irq_table(struct pci_dev
> *pcidev, unsigned int nvec)
>  	return table;
>  }
> 
> +static int find_dfl_in_cfg(struct pci_dev *pcidev,
> +			   struct dfl_fpga_enum_info *info)
> +{
> +	u32 bar, offset, vndr_hdr, dfl_cnt, dfl_res;
> +	int dfl_res_off, i, voff = 0;
> +	resource_size_t start, len;
> +
> +	while ((voff = pci_find_next_ext_capability(pcidev, voff,
> PCI_EXT_CAP_ID_VNDR))) {
> +
> +		pci_read_config_dword(pcidev, voff + PCI_VNDR_HEADER,
> &vndr_hdr);
> +
> +		dev_dbg(&pcidev->dev,
> +			"vendor-specific capability id 0x%x, rev 0x%x len
> 0x%x\n",
> +			PCI_VNDR_HEADER_ID(vndr_hdr),
> +			PCI_VNDR_HEADER_REV(vndr_hdr),
> +			PCI_VNDR_HEADER_LEN(vndr_hdr));

Why you need this log?

> +
> +		if (PCI_VNDR_HEADER_ID(vndr_hdr) == PCI_VNDR_ID_DFLS)
> +			break;
> +	}
> +
> +	if (!voff) {
> +		dev_dbg(&pcidev->dev, "%s no VSEC found\n", __func__);

	"no DFL VSEC found"

> +		return -ENODEV;
> +	}
> +
> +	pci_read_config_dword(pcidev, voff + PCI_VNDR_DFLS_CNT_OFFSET,
> &dfl_cnt);

I believe OFFSET can be removed. : ) 

> +	dev_info(&pcidev->dev, "dfl_cnt %d\n", dfl_cnt);

dev_dbg

> +	for (i = 0; i < dfl_cnt; i++) {
> +		dfl_res_off = voff + PCI_VNDR_DFLS_RES_OFFSET +
> +				      (i * sizeof(dfl_res));
> +		pci_read_config_dword(pcidev, dfl_res_off, &dfl_res);
> +
> +		dev_dbg(&pcidev->dev, "dfl_res 0x%x\n", dfl_res);
> +
> +		bar = dfl_res & PCI_VND_DFLS_RES_BAR_MASK;
> +
> +		if (bar >= PCI_STD_NUM_BARS) {
> +			dev_err(&pcidev->dev, "%s bad bar number %d\n",
> +				__func__, bar);
> +			return -EINVAL;
> +		}
> +
> +		len = pci_resource_len(pcidev, bar);
> +

Remove this blank line.

> +		if (len == 0) {
> +			dev_err(&pcidev->dev, "%s unmapped bar
> number %d\n",

Why "unmapped bar"?

> +				__func__, bar);
> +			return -EINVAL;
> +		}
> +
> +		offset = dfl_res & ~PCI_VND_DFLS_RES_BAR_MASK;
> +

Remove this blank line.

> +		if (offset >= len) {
> +			dev_err(&pcidev->dev, "%s bad offset %u >= %llu\n",
> +				__func__, offset, len);
> +			return -EINVAL;
> +		}
> +
> +		dev_info(&pcidev->dev, "%s BAR %d offset 0x%x\n",
> __func__, bar, offset);

dev_dbg

> +
> +		start = pci_resource_start(pcidev, bar) + offset;
> +		len -= offset;
> +
> +		if (!PAGE_ALIGNED(start)) {

Is this a hard requirement? Or offset should be page aligned per VSEC definition?
Or this is just the requirement from driver point of view. Actually we don't like
to add rules only in driver, so it's better we have this requirement in VSEC definition
with proper documentation.

> +			dev_err(&pcidev->dev, "%s unaliged start 0x%llx\n",

unaligned 

> +				__func__, start);
> +			return -EINVAL;
> +		}
> +
> +		dfl_fpga_enum_info_add_dfl(info, start, len);
> +	}
> +
> +	return 0;
> +}
> +
>  static int find_dfl_in_bar0(struct pci_dev *pcidev,
>  			    struct dfl_fpga_enum_info *info)
>  {
> @@ -221,7 +304,10 @@ static int cci_enumerate_feature_devs(struct
> pci_dev *pcidev)
>  			goto irq_free_exit;
>  	}
> 
> -	ret = find_dfl_in_bar0(pcidev, info);

find_dfl or find_dfl_by_default

Actually the original idea is to hardcode the start of the first DFL per device id.

> +	ret = find_dfl_in_cfg(pcidev, info);

find_dfl_by_vsec

> +

Remove blank line.

> +	if (ret)
> +		ret = find_dfl_in_bar0(pcidev, info);

I am not sure if find_dfl_by_vsec failed, we still can try to find the first dfl in bar0.
Most cases it won't work, especially for multiple DFLs case. Just return error directly.
If someone implements the vsec, it must ensure that vsec contains the correct value,
no fallback solution. Otherwise it doesn't need to implement such vsec, right?

Thanks
Hao

> 
>  	if (ret)
>  		goto irq_free_exit;
> --
> 2.25.2
Wu, Hao Nov. 17, 2020, 10 a.m. UTC | #3
> > +
> > +		start = pci_resource_start(pcidev, bar) + offset;
> > +		len -= offset;
> 
> With these code, I have the following assumption:
> 
> 1. There is only one DFL in one bar, multiple DFLs requires multiple
> bars.
> 
> 2. The DFL region is from the "offset" to the end of the bar.

I think we should not have such kind of limitation, but at least it 
requires user clearly from spec, to make sure no overlap case could
happen. We all know that BAR number is small, but we won't limit
DFL numbers by BAR number here.

> 
> Are they correct? If yes maybe we should specify them clearly in Doc.
> 
> > +
> > +		if (!PAGE_ALIGNED(start)) {
> > +			dev_err(&pcidev->dev, "%s unaliged start 0x%llx\n",
> > +				__func__, start);
> > +			return -EINVAL;
> > +		}
> > +
> > +		dfl_fpga_enum_info_add_dfl(info, start, len);
> 
> Do we need some region overlapping check in this func? So we could find
> the HW problem (e.g. same bar num for multiple DFLs) in early stage.

Not sure if VSEC can add a length field for this purpose, otherwise overlapping
check only can be done after enumeration (walk the DFL to know the end).

> 
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  static int find_dfl_in_bar0(struct pci_dev *pcidev,
> >  			    struct dfl_fpga_enum_info *info)
> >  {
> > @@ -221,7 +304,10 @@ static int cci_enumerate_feature_devs(struct
> pci_dev *pcidev)
> >  			goto irq_free_exit;
> >  	}
> >
> > -	ret = find_dfl_in_bar0(pcidev, info);
> > +	ret = find_dfl_in_cfg(pcidev, info);
> > +
> > +	if (ret)
> > +		ret = find_dfl_in_bar0(pcidev, info);
> 
> The patch is more than the relocation support for DFL. Actually it
> introduced a different way of DFL finding.
> 
> Previously it starts at bar0 offset 0, find dfl fme first, then find
> dfl port according to fme header registers. Now it enumerates every DFL
> by PCIe VSEC.

Yes, the name is a little confusing, maybe we can rename them.

find_dfls_by_default or find_dfls - to handle original cases.
find_dfls_by_vsec - to handle vsec case.

Thanks
Hao

> 
> Maybe we should add more description about the change and why.
> 
> Thanks,
> Yilun
> 
> >
> >  	if (ret)
> >  		goto irq_free_exit;
> > --
> > 2.25.2
kernel test robot Nov. 17, 2020, 2:33 p.m. UTC | #4
Hi,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linux/master]
[also build test WARNING on linus/master v5.10-rc4 next-20201117]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/matthew-gerlach-linux-intel-com/fpga-dfl-optional-VSEC-for-start-of-dfl/20201117-092704
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 09162bc32c880a791c6c0668ce0745cf7958f576
config: xtensa-randconfig-r015-20201116 (attached as .config)
compiler: xtensa-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/b7db30a055d8c6a2cb8fade07b60918c59ecceb7
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review matthew-gerlach-linux-intel-com/fpga-dfl-optional-VSEC-for-start-of-dfl/20201117-092704
        git checkout b7db30a055d8c6a2cb8fade07b60918c59ecceb7
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=xtensa 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from include/linux/device.h:15,
                    from include/linux/pci.h:37,
                    from drivers/fpga/dfl-pci.c:17:
   drivers/fpga/dfl-pci.c: In function 'find_dfl_in_cfg':
>> drivers/fpga/dfl-pci.c:183:26: warning: format '%llu' expects argument of type 'long long unsigned int', but argument 5 has type 'resource_size_t' {aka 'unsigned int'} [-Wformat=]
     183 |    dev_err(&pcidev->dev, "%s bad offset %u >= %llu\n",
         |                          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/dev_printk.h:19:22: note: in definition of macro 'dev_fmt'
      19 | #define dev_fmt(fmt) fmt
         |                      ^~~
   drivers/fpga/dfl-pci.c:183:4: note: in expansion of macro 'dev_err'
     183 |    dev_err(&pcidev->dev, "%s bad offset %u >= %llu\n",
         |    ^~~~~~~
   drivers/fpga/dfl-pci.c:183:50: note: format string is defined here
     183 |    dev_err(&pcidev->dev, "%s bad offset %u >= %llu\n",
         |                                               ~~~^
         |                                                  |
         |                                                  long long unsigned int
         |                                               %u
   In file included from include/linux/device.h:15,
                    from include/linux/pci.h:37,
                    from drivers/fpga/dfl-pci.c:17:
>> drivers/fpga/dfl-pci.c:194:26: warning: format '%llx' expects argument of type 'long long unsigned int', but argument 4 has type 'resource_size_t' {aka 'unsigned int'} [-Wformat=]
     194 |    dev_err(&pcidev->dev, "%s unaliged start 0x%llx\n",
         |                          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/dev_printk.h:19:22: note: in definition of macro 'dev_fmt'
      19 | #define dev_fmt(fmt) fmt
         |                      ^~~
   drivers/fpga/dfl-pci.c:194:4: note: in expansion of macro 'dev_err'
     194 |    dev_err(&pcidev->dev, "%s unaliged start 0x%llx\n",
         |    ^~~~~~~
   drivers/fpga/dfl-pci.c:194:50: note: format string is defined here
     194 |    dev_err(&pcidev->dev, "%s unaliged start 0x%llx\n",
         |                                               ~~~^
         |                                                  |
         |                                                  long long unsigned int
         |                                               %x

vim +183 drivers/fpga/dfl-pci.c

   128	
   129	static int find_dfl_in_cfg(struct pci_dev *pcidev,
   130				   struct dfl_fpga_enum_info *info)
   131	{
   132		u32 bar, offset, vndr_hdr, dfl_cnt, dfl_res;
   133		int dfl_res_off, i, voff = 0;
   134		resource_size_t start, len;
   135	
   136		while ((voff = pci_find_next_ext_capability(pcidev, voff, PCI_EXT_CAP_ID_VNDR))) {
   137	
   138			pci_read_config_dword(pcidev, voff + PCI_VNDR_HEADER, &vndr_hdr);
   139	
   140			dev_dbg(&pcidev->dev,
   141				"vendor-specific capability id 0x%x, rev 0x%x len 0x%x\n",
   142				PCI_VNDR_HEADER_ID(vndr_hdr),
   143				PCI_VNDR_HEADER_REV(vndr_hdr),
   144				PCI_VNDR_HEADER_LEN(vndr_hdr));
   145	
   146			if (PCI_VNDR_HEADER_ID(vndr_hdr) == PCI_VNDR_ID_DFLS)
   147				break;
   148		}
   149	
   150		if (!voff) {
   151			dev_dbg(&pcidev->dev, "%s no VSEC found\n", __func__);
   152			return -ENODEV;
   153		}
   154	
   155		pci_read_config_dword(pcidev, voff + PCI_VNDR_DFLS_CNT_OFFSET, &dfl_cnt);
   156		dev_info(&pcidev->dev, "dfl_cnt %d\n", dfl_cnt);
   157		for (i = 0; i < dfl_cnt; i++) {
   158			dfl_res_off = voff + PCI_VNDR_DFLS_RES_OFFSET +
   159					      (i * sizeof(dfl_res));
   160			pci_read_config_dword(pcidev, dfl_res_off, &dfl_res);
   161	
   162			dev_dbg(&pcidev->dev, "dfl_res 0x%x\n", dfl_res);
   163	
   164			bar = dfl_res & PCI_VND_DFLS_RES_BAR_MASK;
   165	
   166			if (bar >= PCI_STD_NUM_BARS) {
   167				dev_err(&pcidev->dev, "%s bad bar number %d\n",
   168					__func__, bar);
   169				return -EINVAL;
   170			}
   171	
   172			len = pci_resource_len(pcidev, bar);
   173	
   174			if (len == 0) {
   175				dev_err(&pcidev->dev, "%s unmapped bar number %d\n",
   176					__func__, bar);
   177				return -EINVAL;
   178			}
   179	
   180			offset = dfl_res & ~PCI_VND_DFLS_RES_BAR_MASK;
   181	
   182			if (offset >= len) {
 > 183				dev_err(&pcidev->dev, "%s bad offset %u >= %llu\n",
   184					__func__, offset, len);
   185				return -EINVAL;
   186			}
   187	
   188			dev_info(&pcidev->dev, "%s BAR %d offset 0x%x\n", __func__, bar, offset);
   189	
   190			start = pci_resource_start(pcidev, bar) + offset;
   191			len -= offset;
   192	
   193			if (!PAGE_ALIGNED(start)) {
 > 194				dev_err(&pcidev->dev, "%s unaliged start 0x%llx\n",
   195					__func__, start);
   196				return -EINVAL;
   197			}
   198	
   199			dfl_fpga_enum_info_add_dfl(info, start, len);
   200		}
   201	
   202		return 0;
   203	}
   204	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
matthew.gerlach@linux.intel.com Nov. 17, 2020, 7:41 p.m. UTC | #5
On Tue, 17 Nov 2020, Xu Yilun wrote:

> On Mon, Nov 16, 2020 at 05:25:52PM -0800, matthew.gerlach@linux.intel.com wrote:
>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>
>> A DFL may not begin at offset 0 of BAR 0.  A PCIe vendor
>> specific capability can be used to specify the start of a
>> number of DFLs.
>>
>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>> ---
>>  Documentation/fpga/dfl.rst | 10 +++++
>>  drivers/fpga/dfl-pci.c     | 88 +++++++++++++++++++++++++++++++++++++-
>>  2 files changed, 97 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/fpga/dfl.rst b/Documentation/fpga/dfl.rst
>> index 0404fe6ffc74..c81ceb1e79e2 100644
>> --- a/Documentation/fpga/dfl.rst
>> +++ b/Documentation/fpga/dfl.rst
>> @@ -501,6 +501,16 @@ Developer only needs to provide a sub feature driver with matched feature id.
>>  FME Partial Reconfiguration Sub Feature driver (see drivers/fpga/dfl-fme-pr.c)
>>  could be a reference.
>>
>> +Location of DFLs on PCI bus
>> +===========================
>> +The start of the DFL is assumed to be offset 0 of bar 0.
>> +Alternatively, a vendor specific capability structure can be used to
>> +specify the location of one or more DFLs.  Intel has reserved the
>> +vendor specific id of 0x43 for this purpose.  The vendor specific
>> +data begins with a 4 byte count of the number of DFLs followed 4 byte
>> +Offset/BIR fields for each DFL. Bits 2:0 of Offset/BIR field indicates
>> +the BAR, and bits 31:3 form the 8 byte aligned offset where bits 2:0 are
>> +zero.
>>
>>  Open discussion
>>  ===============
>> diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c
>> index b1b157b41942..5418e8bf2496 100644
>> --- a/drivers/fpga/dfl-pci.c
>> +++ b/drivers/fpga/dfl-pci.c
>> @@ -27,6 +27,13 @@
>>  #define DRV_VERSION	"0.8"
>>  #define DRV_NAME	"dfl-pci"
>>
>> +#define PCI_VNDR_ID_DFLS 0x43
>> +
>> +#define PCI_VNDR_DFLS_CNT_OFFSET 8
>> +#define PCI_VNDR_DFLS_RES_OFFSET 0x0c
>> +
>> +#define PCI_VND_DFLS_RES_BAR_MASK 0x7
>
> We could define the mask by GENMASK().
>
> Also another macro PCI_VND_DFLS_RES_OFFSET_MASK is needed.

I will use GENMASK and and add PCI_VND_DFLS_RES_OFFSET_MASK in v2.
>
>> +
>>  struct cci_drvdata {
>>  	struct dfl_fpga_cdev *cdev;	/* container device */
>>  };
>> @@ -119,6 +126,82 @@ static int *cci_pci_create_irq_table(struct pci_dev *pcidev, unsigned int nvec)
>>  	return table;
>>  }
>>
>> +static int find_dfl_in_cfg(struct pci_dev *pcidev,
>> +			   struct dfl_fpga_enum_info *info)
>> +{
>> +	u32 bar, offset, vndr_hdr, dfl_cnt, dfl_res;
>> +	int dfl_res_off, i, voff = 0;
>> +	resource_size_t start, len;
>> +
>> +	while ((voff = pci_find_next_ext_capability(pcidev, voff, PCI_EXT_CAP_ID_VNDR))) {
>> +
>> +		pci_read_config_dword(pcidev, voff + PCI_VNDR_HEADER, &vndr_hdr);
>> +
>> +		dev_dbg(&pcidev->dev,
>> +			"vendor-specific capability id 0x%x, rev 0x%x len 0x%x\n",
>> +			PCI_VNDR_HEADER_ID(vndr_hdr),
>> +			PCI_VNDR_HEADER_REV(vndr_hdr),
>> +			PCI_VNDR_HEADER_LEN(vndr_hdr));
>> +
>> +		if (PCI_VNDR_HEADER_ID(vndr_hdr) == PCI_VNDR_ID_DFLS)
>> +			break;
>> +	}
>> +
>> +	if (!voff) {
>> +		dev_dbg(&pcidev->dev, "%s no VSEC found\n", __func__);
>> +		return -ENODEV;
>> +	}
>> +
>> +	pci_read_config_dword(pcidev, voff + PCI_VNDR_DFLS_CNT_OFFSET, &dfl_cnt);
>> +	dev_info(&pcidev->dev, "dfl_cnt %d\n", dfl_cnt);
>
> dev_dbg() is better?

I will change to dev_dbg in v2.

>
>> +	for (i = 0; i < dfl_cnt; i++) {
>> +		dfl_res_off = voff + PCI_VNDR_DFLS_RES_OFFSET +
>> +				      (i * sizeof(dfl_res));
>> +		pci_read_config_dword(pcidev, dfl_res_off, &dfl_res);
>> +
>> +		dev_dbg(&pcidev->dev, "dfl_res 0x%x\n", dfl_res);
>> +
>> +		bar = dfl_res & PCI_VND_DFLS_RES_BAR_MASK;
>
> FIELD_GET is better?

I think & will the GENMASK will be better because it will be
symetrical to the & below for the offset.

>
>> +
>> +		if (bar >= PCI_STD_NUM_BARS) {
>> +			dev_err(&pcidev->dev, "%s bad bar number %d\n",
>> +				__func__, bar);
>> +			return -EINVAL;
>> +		}
>> +
>> +		len = pci_resource_len(pcidev, bar);
>> +
>> +		if (len == 0) {
>> +			dev_err(&pcidev->dev, "%s unmapped bar number %d\n",
>> +				__func__, bar);
>> +			return -EINVAL;
>> +		}
>> +
>> +		offset = dfl_res & ~PCI_VND_DFLS_RES_BAR_MASK;
>
> ditto
We don't want to use FIELD_GET here because we don't the shifting.

>
>> +
>> +		if (offset >= len) {
>> +			dev_err(&pcidev->dev, "%s bad offset %u >= %llu\n",
>> +				__func__, offset, len);
>> +			return -EINVAL;
>> +		}
>> +
>> +		dev_info(&pcidev->dev, "%s BAR %d offset 0x%x\n", __func__, bar, offset);
>
> dev_dbg()?

I will change to dev_dbg in v2.

>
>> +
>> +		start = pci_resource_start(pcidev, bar) + offset;
>> +		len -= offset;
>
> With these code, I have the following assumption:
>
> 1. There is only one DFL in one bar, multiple DFLs requires multiple
> bars.
>
> 2. The DFL region is from the "offset" to the end of the bar.
>
> Are they correct? If yes maybe we should specify them clearly in Doc.
>

This code would have the same assumptions as the existing code for finding 
the dfls.  The len value is only used during the walk of the DFL to 
prevent walking too far.  So I think one could have more than one DFL
on a particular bar as long as the start of the DFLs are different.

>> +
>> +		if (!PAGE_ALIGNED(start)) {
>> +			dev_err(&pcidev->dev, "%s unaliged start 0x%llx\n",
>> +				__func__, start);
>> +			return -EINVAL;
>> +		}
>> +
>> +		dfl_fpga_enum_info_add_dfl(info, start, len);
>
> Do we need some region overlapping check in this func? So we could find
> the HW problem (e.g. same bar num for multiple DFLs) in early stage.
>

I think whatever overlapping check would also need to be in the existing 
code because the logic is the same.

>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>  static int find_dfl_in_bar0(struct pci_dev *pcidev,
>>  			    struct dfl_fpga_enum_info *info)
>>  {
>> @@ -221,7 +304,10 @@ static int cci_enumerate_feature_devs(struct pci_dev *pcidev)
>>  			goto irq_free_exit;
>>  	}
>>
>> -	ret = find_dfl_in_bar0(pcidev, info);
>> +	ret = find_dfl_in_cfg(pcidev, info);
>> +
>> +	if (ret)
>> +		ret = find_dfl_in_bar0(pcidev, info);
>
> The patch is more than the relocation support for DFL. Actually it
> introduced a different way of DFL finding.
>
> Previously it starts at bar0 offset 0, find dfl fme first, then find
> dfl port according to fme header registers. Now it enumerates every DFL
> by PCIe VSEC.
>
> Maybe we should add more description about the change and why.

I will highlight this difference in the documentation in v2.
>
> Thanks,
> Yilun
>
>>
>>  	if (ret)
>>  		goto irq_free_exit;
>> --
>> 2.25.2
>
matthew.gerlach@linux.intel.com Nov. 17, 2020, 8:09 p.m. UTC | #6
On Tue, 17 Nov 2020, Wu, Hao wrote:

>> Subject: [PATCH 2/2] fpga: dfl: look for vendor specific capability
>>
>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>
>> A DFL may not begin at offset 0 of BAR 0.  A PCIe vendor
>> specific capability can be used to specify the start of a
>> number of DFLs.
>>
>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>> ---
>>  Documentation/fpga/dfl.rst | 10 +++++
>>  drivers/fpga/dfl-pci.c     | 88 +++++++++++++++++++++++++++++++++++++-
>>  2 files changed, 97 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/fpga/dfl.rst b/Documentation/fpga/dfl.rst
>> index 0404fe6ffc74..c81ceb1e79e2 100644
>> --- a/Documentation/fpga/dfl.rst
>> +++ b/Documentation/fpga/dfl.rst
>> @@ -501,6 +501,16 @@ Developer only needs to provide a sub feature
>> driver with matched feature id.
>>  FME Partial Reconfiguration Sub Feature driver (see drivers/fpga/dfl-fme-
>> pr.c)
>>  could be a reference.
>>
>> +Location of DFLs on PCI bus
>
> Location of DFLs on PCI Device

I will update in v2.
>
>> +===========================
>> +The start of the DFL is assumed to be offset 0 of bar 0.
>
> the first DFL

I will make this change in v2.

>
>> +Alternatively, a vendor specific capability structure can be used to
>> +specify the location of one or more DFLs.  Intel has reserved the
>
> I believe this capability is used to specify all DFLs on this the PCI device.

Yes, Yilun noticed this as well.  Documentation will be updated in v2.

>
>> +vendor specific id of 0x43 for this purpose.  The vendor specific
>
> VSEC ID is 0x43.

OK will change in v2.

>
> One more question here is, does it require vendor id to be intel firstly?
> Or other vendors could implement the same one but with a different id?

As implemented, the PCI Vendor is not examined.

>> +data begins with a 4 byte count of the number of DFLs followed 4 byte
>
> vendor specific register

Updated in v2.
>
>> +Offset/BIR fields for each DFL. Bits 2:0 of Offset/BIR field indicates
>> +the BAR, and bits 31:3 form the 8 byte aligned offset where bits 2:0 are
>> +zero.
>>
>>  Open discussion
>>  ===============
>> diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c
>> index b1b157b41942..5418e8bf2496 100644
>> --- a/drivers/fpga/dfl-pci.c
>> +++ b/drivers/fpga/dfl-pci.c
>> @@ -27,6 +27,13 @@
>>  #define DRV_VERSION	"0.8"
>>  #define DRV_NAME	"dfl-pci"
>>
>> +#define PCI_VNDR_ID_DFLS 0x43
>
> What about PCI_VSEC_ID_INTEL_DFLS?
>
> Is it possible a different ID chosen by different vendor?

I think another vendor could choose their own ID.

>
>> +
>> +#define PCI_VNDR_DFLS_CNT_OFFSET 8
>> +#define PCI_VNDR_DFLS_RES_OFFSET 0x0c
>
> What about VSEC_DFLS_CNT ?

I will remove _OFFSET in v2.

>
>> +
>> +#define PCI_VND_DFLS_RES_BAR_MASK 0x7
>> +
>>  struct cci_drvdata {
>>  	struct dfl_fpga_cdev *cdev;	/* container device */
>>  };
>> @@ -119,6 +126,82 @@ static int *cci_pci_create_irq_table(struct pci_dev
>> *pcidev, unsigned int nvec)
>>  	return table;
>>  }
>>
>> +static int find_dfl_in_cfg(struct pci_dev *pcidev,
>> +			   struct dfl_fpga_enum_info *info)
>> +{
>> +	u32 bar, offset, vndr_hdr, dfl_cnt, dfl_res;
>> +	int dfl_res_off, i, voff = 0;
>> +	resource_size_t start, len;
>> +
>> +	while ((voff = pci_find_next_ext_capability(pcidev, voff,
>> PCI_EXT_CAP_ID_VNDR))) {
>> +
>> +		pci_read_config_dword(pcidev, voff + PCI_VNDR_HEADER,
>> &vndr_hdr);
>> +
>> +		dev_dbg(&pcidev->dev,
>> +			"vendor-specific capability id 0x%x, rev 0x%x len
>> 0x%x\n",
>> +			PCI_VNDR_HEADER_ID(vndr_hdr),
>> +			PCI_VNDR_HEADER_REV(vndr_hdr),
>> +			PCI_VNDR_HEADER_LEN(vndr_hdr));
>
> Why you need this log?

For debugging.

>
>> +
>> +		if (PCI_VNDR_HEADER_ID(vndr_hdr) == PCI_VNDR_ID_DFLS)
>> +			break;
>> +	}
>> +
>> +	if (!voff) {
>> +		dev_dbg(&pcidev->dev, "%s no VSEC found\n", __func__);
>
> 	"no DFL VSEC found"
>
>> +		return -ENODEV;
>> +	}
>> +
>> +	pci_read_config_dword(pcidev, voff + PCI_VNDR_DFLS_CNT_OFFSET,
>> &dfl_cnt);
>
> I believe OFFSET can be removed. : )

Yes, _OFFSET to be removed in v2.

>
>> +	dev_info(&pcidev->dev, "dfl_cnt %d\n", dfl_cnt);
>
> dev_dbg

Yup, v2.
>
>> +	for (i = 0; i < dfl_cnt; i++) {
>> +		dfl_res_off = voff + PCI_VNDR_DFLS_RES_OFFSET +
>> +				      (i * sizeof(dfl_res));
>> +		pci_read_config_dword(pcidev, dfl_res_off, &dfl_res);
>> +
>> +		dev_dbg(&pcidev->dev, "dfl_res 0x%x\n", dfl_res);
>> +
>> +		bar = dfl_res & PCI_VND_DFLS_RES_BAR_MASK;
>> +
>> +		if (bar >= PCI_STD_NUM_BARS) {
>> +			dev_err(&pcidev->dev, "%s bad bar number %d\n",
>> +				__func__, bar);
>> +			return -EINVAL;
>> +		}
>> +
>> +		len = pci_resource_len(pcidev, bar);
>> +
>
> Remove this blank line.
OK, v2.

>
>> +		if (len == 0) {
>> +			dev_err(&pcidev->dev, "%s unmapped bar
>> number %d\n",
>
> Why "unmapped bar"?

How about, "zero length bar"?

>
>> +				__func__, bar);
>> +			return -EINVAL;
>> +		}
>> +
>> +		offset = dfl_res & ~PCI_VND_DFLS_RES_BAR_MASK;
>> +
>
> Remove this blank line.

OK, v2.

>
>> +		if (offset >= len) {
>> +			dev_err(&pcidev->dev, "%s bad offset %u >= %llu\n",
>> +				__func__, offset, len);
>> +			return -EINVAL;
>> +		}
>> +
>> +		dev_info(&pcidev->dev, "%s BAR %d offset 0x%x\n",
>> __func__, bar, offset);
>
> dev_dbg

v2

>
>> +
>> +		start = pci_resource_start(pcidev, bar) + offset;
>> +		len -= offset;
>> +
>> +		if (!PAGE_ALIGNED(start)) {
>
> Is this a hard requirement? Or offset should be page aligned per VSEC definition?
> Or this is just the requirement from driver point of view. Actually we don't like
> to add rules only in driver, so it's better we have this requirement in VSEC definition
> with proper documentation.

The DFL parsing code ioremaps the memory bounded by start/len.  I thought 
this would require the start to be page aligned.

>
>> +			dev_err(&pcidev->dev, "%s unaliged start 0x%llx\n",
>
> unaligned

v2.

>
>> +				__func__, start);
>> +			return -EINVAL;
>> +		}
>> +
>> +		dfl_fpga_enum_info_add_dfl(info, start, len);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>  static int find_dfl_in_bar0(struct pci_dev *pcidev,
>>  			    struct dfl_fpga_enum_info *info)
>>  {
>> @@ -221,7 +304,10 @@ static int cci_enumerate_feature_devs(struct
>> pci_dev *pcidev)
>>  			goto irq_free_exit;
>>  	}
>>
>> -	ret = find_dfl_in_bar0(pcidev, info);
>
> find_dfl or find_dfl_by_default
>
> Actually the original idea is to hardcode the start of the first DFL per device id.
>
>> +	ret = find_dfl_in_cfg(pcidev, info);
>
> find_dfl_by_vsec

Adopting find_dfls_by_vsec and find_dfls_by_default in v2.

>
>> +
>
> Remove blank line.

Done in v2.
>
>> +	if (ret)
>> +		ret = find_dfl_in_bar0(pcidev, info);
>
> I am not sure if find_dfl_by_vsec failed, we still can try to find the first dfl in bar0.
> Most cases it won't work, especially for multiple DFLs case. Just return error directly.
> If someone implements the vsec, it must ensure that vsec contains the correct value,
> no fallback solution. Otherwise it doesn't need to implement such vsec, right?

If no VSEC is found, find_dfls_by_vsec will return -ENODEV.  Will update 
to (ret == -ENODEV) in v2.

>
> Thanks
> Hao
>
>>
>>  	if (ret)
>>  		goto irq_free_exit;
>> --
>> 2.25.2
>
>
Tom Rix Nov. 17, 2020, 8:35 p.m. UTC | #7
On 11/16/20 5:25 PM, matthew.gerlach@linux.intel.com wrote:
> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>
> A DFL may not begin at offset 0 of BAR 0.  A PCIe vendor
> specific capability can be used to specify the start of a
> number of DFLs.
>
> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> ---
>  Documentation/fpga/dfl.rst | 10 +++++
>  drivers/fpga/dfl-pci.c     | 88 +++++++++++++++++++++++++++++++++++++-
>  2 files changed, 97 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/fpga/dfl.rst b/Documentation/fpga/dfl.rst
> index 0404fe6ffc74..c81ceb1e79e2 100644
> --- a/Documentation/fpga/dfl.rst
> +++ b/Documentation/fpga/dfl.rst
> @@ -501,6 +501,16 @@ Developer only needs to provide a sub feature driver with matched feature id.
>  FME Partial Reconfiguration Sub Feature driver (see drivers/fpga/dfl-fme-pr.c)
>  could be a reference.
>  
> +Location of DFLs on PCI bus
> +===========================
> +The start of the DFL is assumed to be offset 0 of bar 0.
> +Alternatively, a vendor specific capability structure can be used to
> +specify the location of one or more DFLs.  Intel has reserved the
> +vendor specific id of 0x43 for this purpose.  The vendor specific
> +data begins with a 4 byte count of the number of DFLs followed 4 byte
> +Offset/BIR fields for each DFL. Bits 2:0 of Offset/BIR field indicates
> +the BAR, and bits 31:3 form the 8 byte aligned offset where bits 2:0 are
> +zero.
>  

Does the 'Device Feature List (DFL) Overview' section need to change ?

Maybe some more ascii art on location of bar0 vs vendor specific ?

>  Open discussion
>  ===============
> diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c
> index b1b157b41942..5418e8bf2496 100644
> --- a/drivers/fpga/dfl-pci.c
> +++ b/drivers/fpga/dfl-pci.c
> @@ -27,6 +27,13 @@
>  #define DRV_VERSION	"0.8"
Since basic pci functionality is changing, consider incrementing this version.
>  #define DRV_NAME	"dfl-pci"
>  
> +#define PCI_VNDR_ID_DFLS 0x43
> +
> +#define PCI_VNDR_DFLS_CNT_OFFSET 8
> +#define PCI_VNDR_DFLS_RES_OFFSET 0x0c
> +
> +#define PCI_VND_DFLS_RES_BAR_MASK 0x7
Is this missing a R? PCI_VNDR_DFLS_RES_BAR_MASK ?
> +
>  struct cci_drvdata {
>  	struct dfl_fpga_cdev *cdev;	/* container device */
>  };
> @@ -119,6 +126,82 @@ static int *cci_pci_create_irq_table(struct pci_dev *pcidev, unsigned int nvec)
>  	return table;
>  }
>  
> +static int find_dfl_in_cfg(struct pci_dev *pcidev,
> +			   struct dfl_fpga_enum_info *info)
> +{
> +	u32 bar, offset, vndr_hdr, dfl_cnt, dfl_res;
> +	int dfl_res_off, i, voff = 0;
> +	resource_size_t start, len;
> +
> +	while ((voff = pci_find_next_ext_capability(pcidev, voff, PCI_EXT_CAP_ID_VNDR))) {
> +
extra nl
> +		pci_read_config_dword(pcidev, voff + PCI_VNDR_HEADER, &vndr_hdr);

A general problem.

Return of pci_read is not checked, nor are the values ex/ vndr_hdr initialized.

> +
> +		dev_dbg(&pcidev->dev,
> +			"vendor-specific capability id 0x%x, rev 0x%x len 0x%x\n",
> +			PCI_VNDR_HEADER_ID(vndr_hdr),
> +			PCI_VNDR_HEADER_REV(vndr_hdr),
> +			PCI_VNDR_HEADER_LEN(vndr_hdr));
> +
> +		if (PCI_VNDR_HEADER_ID(vndr_hdr) == PCI_VNDR_ID_DFLS)
> +			break;
> +	}
> +
> +	if (!voff) {
> +		dev_dbg(&pcidev->dev, "%s no VSEC found\n", __func__);
> +		return -ENODEV;
> +	}
> +
> +	pci_read_config_dword(pcidev, voff + PCI_VNDR_DFLS_CNT_OFFSET, &dfl_cnt);
> +	dev_info(&pcidev->dev, "dfl_cnt %d\n", dfl_cnt);
> +	for (i = 0; i < dfl_cnt; i++) {
Is there a upper limit on the dfl_cnt ? maybe PCI_STD_NUM_BARS ?
> +		dfl_res_off = voff + PCI_VNDR_DFLS_RES_OFFSET +
> +				      (i * sizeof(dfl_res));
> +		pci_read_config_dword(pcidev, dfl_res_off, &dfl_res);
> +
> +		dev_dbg(&pcidev->dev, "dfl_res 0x%x\n", dfl_res);
> +
> +		bar = dfl_res & PCI_VND_DFLS_RES_BAR_MASK;
an extra nl, fix the similar ones as well.
> +
> +		if (bar >= PCI_STD_NUM_BARS) {
> +			dev_err(&pcidev->dev, "%s bad bar number %d\n",
> +				__func__, bar);
> +			return -EINVAL;
> +		}
> +
> +		len = pci_resource_len(pcidev, bar);
> +
> +		if (len == 0) {
> +			dev_err(&pcidev->dev, "%s unmapped bar number %d\n",
> +				__func__, bar);
> +			return -EINVAL;
> +		}
> +
> +		offset = dfl_res & ~PCI_VND_DFLS_RES_BAR_MASK;
> +
> +		if (offset >= len) {
> +			dev_err(&pcidev->dev, "%s bad offset %u >= %llu\n",
> +				__func__, offset, len);
> +			return -EINVAL;
> +		}
> +
> +		dev_info(&pcidev->dev, "%s BAR %d offset 0x%x\n", __func__, bar, offset);
> +
> +		start = pci_resource_start(pcidev, bar) + offset;
> +		len -= offset;
> +
> +		if (!PAGE_ALIGNED(start)) {
> +			dev_err(&pcidev->dev, "%s unaliged start 0x%llx\n",
> +				__func__, start);
> +			return -EINVAL;
> +		}
> +
> +		dfl_fpga_enum_info_add_dfl(info, start, len);
> +	}
> +
> +	return 0;
> +}
> +
>  static int find_dfl_in_bar0(struct pci_dev *pcidev,
>  			    struct dfl_fpga_enum_info *info)
>  {
> @@ -221,7 +304,10 @@ static int cci_enumerate_feature_devs(struct pci_dev *pcidev)
>  			goto irq_free_exit;
>  	}
>  
> -	ret = find_dfl_in_bar0(pcidev, info);
> +	ret = find_dfl_in_cfg(pcidev, info);
> +
> +	if (ret)
> +		ret = find_dfl_in_bar0(pcidev, info);

Is this really an either/or ?

Could there be a base functionality on bar0 and a skew functionality on vendor bars?

If vendor is going to completely override, why not use bar0 ?

Tom

>  
>  	if (ret)
>  		goto irq_free_exit;
matthew.gerlach@linux.intel.com Nov. 17, 2020, 11:10 p.m. UTC | #8
On Tue, 17 Nov 2020, Tom Rix wrote:

>
> On 11/16/20 5:25 PM, matthew.gerlach@linux.intel.com wrote:
>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>
>> A DFL may not begin at offset 0 of BAR 0.  A PCIe vendor
>> specific capability can be used to specify the start of a
>> number of DFLs.
>>
>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>> ---
>>  Documentation/fpga/dfl.rst | 10 +++++
>>  drivers/fpga/dfl-pci.c     | 88 +++++++++++++++++++++++++++++++++++++-
>>  2 files changed, 97 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/fpga/dfl.rst b/Documentation/fpga/dfl.rst
>> index 0404fe6ffc74..c81ceb1e79e2 100644
>> --- a/Documentation/fpga/dfl.rst
>> +++ b/Documentation/fpga/dfl.rst
>> @@ -501,6 +501,16 @@ Developer only needs to provide a sub feature driver with matched feature id.
>>  FME Partial Reconfiguration Sub Feature driver (see drivers/fpga/dfl-fme-pr.c)
>>  could be a reference.
>>
>> +Location of DFLs on PCI bus
>> +===========================
>> +The start of the DFL is assumed to be offset 0 of bar 0.
>> +Alternatively, a vendor specific capability structure can be used to
>> +specify the location of one or more DFLs.  Intel has reserved the
>> +vendor specific id of 0x43 for this purpose.  The vendor specific
>> +data begins with a 4 byte count of the number of DFLs followed 4 byte
>> +Offset/BIR fields for each DFL. Bits 2:0 of Offset/BIR field indicates
>> +the BAR, and bits 31:3 form the 8 byte aligned offset where bits 2:0 are
>> +zero.
>>
>
> Does the 'Device Feature List (DFL) Overview' section need to change ?

The 'Device Feature List (DFL) Overview' section does not really mention
the starting location of the DFLs.  I think a section on the discussing
the starting location is enough.

>
> Maybe some more ascii art on location of bar0 vs vendor specific ?

I've added some clarity in v2 which might be enough.

>
>>  Open discussion
>>  ===============
>> diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c
>> index b1b157b41942..5418e8bf2496 100644
>> --- a/drivers/fpga/dfl-pci.c
>> +++ b/drivers/fpga/dfl-pci.c
>> @@ -27,6 +27,13 @@
>>  #define DRV_VERSION	"0.8"
> Since basic pci functionality is changing, consider incrementing this version.
>>  #define DRV_NAME	"dfl-pci"
>>
>> +#define PCI_VNDR_ID_DFLS 0x43
>> +
>> +#define PCI_VNDR_DFLS_CNT_OFFSET 8
>> +#define PCI_VNDR_DFLS_RES_OFFSET 0x0c
>> +
>> +#define PCI_VND_DFLS_RES_BAR_MASK 0x7
> Is this missing a R? PCI_VNDR_DFLS_RES_BAR_MASK ?

Good catch!.  Will fix in v2.

>> +
>>  struct cci_drvdata {
>>  	struct dfl_fpga_cdev *cdev;	/* container device */
>>  };
>> @@ -119,6 +126,82 @@ static int *cci_pci_create_irq_table(struct pci_dev *pcidev, unsigned int nvec)
>>  	return table;
>>  }
>>
>> +static int find_dfl_in_cfg(struct pci_dev *pcidev,
>> +			   struct dfl_fpga_enum_info *info)
>> +{
>> +	u32 bar, offset, vndr_hdr, dfl_cnt, dfl_res;
>> +	int dfl_res_off, i, voff = 0;
>> +	resource_size_t start, len;
>> +
>> +	while ((voff = pci_find_next_ext_capability(pcidev, voff, PCI_EXT_CAP_ID_VNDR))) {
>> +
> extra nl
Ok, fix in v2.

>> +		pci_read_config_dword(pcidev, voff + PCI_VNDR_HEADER, &vndr_hdr);
>
> A general problem.
>
> Return of pci_read is not checked, nor are the values ex/ vndr_hdr initialized.

In v2 the variables will be initialized to invalid values that will be 
caught with the existing checks.

>
>> +
>> +		dev_dbg(&pcidev->dev,
>> +			"vendor-specific capability id 0x%x, rev 0x%x len 0x%x\n",
>> +			PCI_VNDR_HEADER_ID(vndr_hdr),
>> +			PCI_VNDR_HEADER_REV(vndr_hdr),
>> +			PCI_VNDR_HEADER_LEN(vndr_hdr));
>> +
>> +		if (PCI_VNDR_HEADER_ID(vndr_hdr) == PCI_VNDR_ID_DFLS)
>> +			break;
>> +	}
>> +
>> +	if (!voff) {
>> +		dev_dbg(&pcidev->dev, "%s no VSEC found\n", __func__);
>> +		return -ENODEV;
>> +	}
>> +
>> +	pci_read_config_dword(pcidev, voff + PCI_VNDR_DFLS_CNT_OFFSET, &dfl_cnt);
>> +	dev_info(&pcidev->dev, "dfl_cnt %d\n", dfl_cnt);
>> +	for (i = 0; i < dfl_cnt; i++) {
> Is there a upper limit on the dfl_cnt ? maybe PCI_STD_NUM_BARS ?

Technically, there could be more than one DFL in a bar.  I don't
really know what criteria constitutes an upper limit.

>> +		dfl_res_off = voff + PCI_VNDR_DFLS_RES_OFFSET +
>> +				      (i * sizeof(dfl_res));
>> +		pci_read_config_dword(pcidev, dfl_res_off, &dfl_res);
>> +
>> +		dev_dbg(&pcidev->dev, "dfl_res 0x%x\n", dfl_res);
>> +
>> +		bar = dfl_res & PCI_VND_DFLS_RES_BAR_MASK;
> an extra nl, fix the similar ones as well.
>> +
>> +		if (bar >= PCI_STD_NUM_BARS) {
>> +			dev_err(&pcidev->dev, "%s bad bar number %d\n",
>> +				__func__, bar);
>> +			return -EINVAL;
>> +		}
>> +
>> +		len = pci_resource_len(pcidev, bar);
>> +
>> +		if (len == 0) {
>> +			dev_err(&pcidev->dev, "%s unmapped bar number %d\n",
>> +				__func__, bar);
>> +			return -EINVAL;
>> +		}
>> +
>> +		offset = dfl_res & ~PCI_VND_DFLS_RES_BAR_MASK;
>> +
>> +		if (offset >= len) {
>> +			dev_err(&pcidev->dev, "%s bad offset %u >= %llu\n",
>> +				__func__, offset, len);
>> +			return -EINVAL;
>> +		}
>> +
>> +		dev_info(&pcidev->dev, "%s BAR %d offset 0x%x\n", __func__, bar, offset);
>> +
>> +		start = pci_resource_start(pcidev, bar) + offset;
>> +		len -= offset;
>> +
>> +		if (!PAGE_ALIGNED(start)) {
>> +			dev_err(&pcidev->dev, "%s unaliged start 0x%llx\n",
>> +				__func__, start);
>> +			return -EINVAL;
>> +		}
>> +
>> +		dfl_fpga_enum_info_add_dfl(info, start, len);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>  static int find_dfl_in_bar0(struct pci_dev *pcidev,
>>  			    struct dfl_fpga_enum_info *info)
>>  {
>> @@ -221,7 +304,10 @@ static int cci_enumerate_feature_devs(struct pci_dev *pcidev)
>>  			goto irq_free_exit;
>>  	}
>>
>> -	ret = find_dfl_in_bar0(pcidev, info);
>> +	ret = find_dfl_in_cfg(pcidev, info);
>> +
>> +	if (ret)
>> +		ret = find_dfl_in_bar0(pcidev, info);
>
> Is this really an either/or ?
>
> Could there be a base functionality on bar0 and a skew functionality on vendor bars?

For simplicity I think either or is better.  If skew functionality is in 
vendor bars, why not just use the vendor bars all the time.

>
> If vendor is going to completely override, why not use bar0 ?

I'm not sure I understand the question, but in v2 the legacy DFL search 
will only occur if there is no VSEC found.

>
> Tom
>
>>
>>  	if (ret)
>>  		goto irq_free_exit;
>
>
Tom Rix Nov. 18, 2020, 1:29 a.m. UTC | #9
>> Is this really an either/or ?
>>
>> Could there be a base functionality on bar0 and a skew functionality on vendor bars?
>
> For simplicity I think either or is better.  If skew functionality is in vendor bars, why not just use the vendor bars all the time.
>
>>
>> If vendor is going to completely override, why not use bar0 ?
>
> I'm not sure I understand the question, but in v2 the legacy DFL search will only occur if there is no VSEC found.
>
Wondering if vsec was ignored, would the bar0 work ?

Or another way, how badly would an old driver behave.

consider incrementing the driver version if it would be bad.

Tom


>>
>> Tom
>>
>>>
>>>      if (ret)
>>>          goto irq_free_exit;
>>
>>
>
Wu, Hao Nov. 18, 2020, 1:54 a.m. UTC | #10
> On Tue, 17 Nov 2020, Wu, Hao wrote:

[...]

> >>  Open discussion
> >>  ===============
> >> diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c
> >> index b1b157b41942..5418e8bf2496 100644
> >> --- a/drivers/fpga/dfl-pci.c
> >> +++ b/drivers/fpga/dfl-pci.c
> >> @@ -27,6 +27,13 @@
> >>  #define DRV_VERSION	"0.8"
> >>  #define DRV_NAME	"dfl-pci"
> >>
> >> +#define PCI_VNDR_ID_DFLS 0x43
> >
> > What about PCI_VSEC_ID_INTEL_DFLS?
> >
> > Is it possible a different ID chosen by different vendor?
> 
> I think another vendor could choose their own ID.

If another vendor could choose their own ID, so should we
check vendor id as well?

[...]

> >> +	for (i = 0; i < dfl_cnt; i++) {
> >> +		dfl_res_off = voff + PCI_VNDR_DFLS_RES_OFFSET +
> >> +				      (i * sizeof(dfl_res));
> >> +		pci_read_config_dword(pcidev, dfl_res_off, &dfl_res);
> >> +
> >> +		dev_dbg(&pcidev->dev, "dfl_res 0x%x\n", dfl_res);
> >> +
> >> +		bar = dfl_res & PCI_VND_DFLS_RES_BAR_MASK;
> >> +
> >> +		if (bar >= PCI_STD_NUM_BARS) {
> >> +			dev_err(&pcidev->dev, "%s bad bar number %d\n",
> >> +				__func__, bar);
> >> +			return -EINVAL;
> >> +		}
> >> +
> >> +		len = pci_resource_len(pcidev, bar);
> >> +
> >
> > Remove this blank line.
> OK, v2.
> 
> >
> >> +		if (len == 0) {
> >> +			dev_err(&pcidev->dev, "%s unmapped bar
> >> number %d\n",
> >
> > Why "unmapped bar"?
> 
> How about, "zero length bar"?

I think this checking can be covered by below one, right?
if (offset >= len)
...

> 
> >
> >> +				__func__, bar);
> >> +			return -EINVAL;
> >> +		}
> >> +
> >> +		offset = dfl_res & ~PCI_VND_DFLS_RES_BAR_MASK;
> >> +
> >
> > Remove this blank line.
> 
> OK, v2.
> 
> >
> >> +		if (offset >= len) {
> >> +			dev_err(&pcidev->dev, "%s bad offset %u >= %llu\n",
> >> +				__func__, offset, len);
> >> +			return -EINVAL;
> >> +		}
> >> +

[....]

> >> +
> >> +		start = pci_resource_start(pcidev, bar) + offset;
> >> +		len -= offset;
> >> +
> >> +		if (!PAGE_ALIGNED(start)) {
> >
> > Is this a hard requirement? Or offset should be page aligned per VSEC
> definition?
> > Or this is just the requirement from driver point of view. Actually we don't
> like
> > to add rules only in driver, so it's better we have this requirement in VSEC
> definition
> > with proper documentation.
> 
> The DFL parsing code ioremaps the memory bounded by start/len.  I thought
> this would require the start to be page aligned.

If consider mmap the region to userspace, it requires page aligned, but do we
need to apply this rule for everyone?

Thanks
Hao
Xu Yilun Nov. 18, 2020, 6:19 a.m. UTC | #11
On Tue, Nov 17, 2020 at 11:41:32AM -0800, matthew.gerlach@linux.intel.com wrote:
> 
> 
> On Tue, 17 Nov 2020, Xu Yilun wrote:
> 
> >On Mon, Nov 16, 2020 at 05:25:52PM -0800, matthew.gerlach@linux.intel.com wrote:
> >>From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> >>
> >>A DFL may not begin at offset 0 of BAR 0.  A PCIe vendor
> >>specific capability can be used to specify the start of a
> >>number of DFLs.
> >>
> >>Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> >>---
> >> Documentation/fpga/dfl.rst | 10 +++++
> >> drivers/fpga/dfl-pci.c     | 88 +++++++++++++++++++++++++++++++++++++-
> >> 2 files changed, 97 insertions(+), 1 deletion(-)
> >>
> >>diff --git a/Documentation/fpga/dfl.rst b/Documentation/fpga/dfl.rst
> >>index 0404fe6ffc74..c81ceb1e79e2 100644
> >>--- a/Documentation/fpga/dfl.rst
> >>+++ b/Documentation/fpga/dfl.rst
> >>@@ -501,6 +501,16 @@ Developer only needs to provide a sub feature driver with matched feature id.
> >> FME Partial Reconfiguration Sub Feature driver (see drivers/fpga/dfl-fme-pr.c)
> >> could be a reference.
> >>
> >>+Location of DFLs on PCI bus
> >>+===========================
> >>+The start of the DFL is assumed to be offset 0 of bar 0.
> >>+Alternatively, a vendor specific capability structure can be used to
> >>+specify the location of one or more DFLs.  Intel has reserved the
> >>+vendor specific id of 0x43 for this purpose.  The vendor specific
> >>+data begins with a 4 byte count of the number of DFLs followed 4 byte
> >>+Offset/BIR fields for each DFL. Bits 2:0 of Offset/BIR field indicates
> >>+the BAR, and bits 31:3 form the 8 byte aligned offset where bits 2:0 are
> >>+zero.
> >>
> >> Open discussion
> >> ===============
> >>diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c
> >>index b1b157b41942..5418e8bf2496 100644
> >>--- a/drivers/fpga/dfl-pci.c
> >>+++ b/drivers/fpga/dfl-pci.c
> >>@@ -27,6 +27,13 @@
> >> #define DRV_VERSION	"0.8"
> >> #define DRV_NAME	"dfl-pci"
> >>
> >>+#define PCI_VNDR_ID_DFLS 0x43
> >>+
> >>+#define PCI_VNDR_DFLS_CNT_OFFSET 8
> >>+#define PCI_VNDR_DFLS_RES_OFFSET 0x0c
> >>+
> >>+#define PCI_VND_DFLS_RES_BAR_MASK 0x7
> >
> >We could define the mask by GENMASK().
> >
> >Also another macro PCI_VND_DFLS_RES_OFFSET_MASK is needed.
> 
> I will use GENMASK and and add PCI_VND_DFLS_RES_OFFSET_MASK in v2.
> >
> >>+
> >> struct cci_drvdata {
> >> 	struct dfl_fpga_cdev *cdev;	/* container device */
> >> };
> >>@@ -119,6 +126,82 @@ static int *cci_pci_create_irq_table(struct pci_dev *pcidev, unsigned int nvec)
> >> 	return table;
> >> }
> >>
> >>+static int find_dfl_in_cfg(struct pci_dev *pcidev,
> >>+			   struct dfl_fpga_enum_info *info)
> >>+{
> >>+	u32 bar, offset, vndr_hdr, dfl_cnt, dfl_res;
> >>+	int dfl_res_off, i, voff = 0;
> >>+	resource_size_t start, len;
> >>+
> >>+	while ((voff = pci_find_next_ext_capability(pcidev, voff, PCI_EXT_CAP_ID_VNDR))) {
> >>+
> >>+		pci_read_config_dword(pcidev, voff + PCI_VNDR_HEADER, &vndr_hdr);
> >>+
> >>+		dev_dbg(&pcidev->dev,
> >>+			"vendor-specific capability id 0x%x, rev 0x%x len 0x%x\n",
> >>+			PCI_VNDR_HEADER_ID(vndr_hdr),
> >>+			PCI_VNDR_HEADER_REV(vndr_hdr),
> >>+			PCI_VNDR_HEADER_LEN(vndr_hdr));
> >>+
> >>+		if (PCI_VNDR_HEADER_ID(vndr_hdr) == PCI_VNDR_ID_DFLS)
> >>+			break;
> >>+	}
> >>+
> >>+	if (!voff) {
> >>+		dev_dbg(&pcidev->dev, "%s no VSEC found\n", __func__);
> >>+		return -ENODEV;
> >>+	}
> >>+
> >>+	pci_read_config_dword(pcidev, voff + PCI_VNDR_DFLS_CNT_OFFSET, &dfl_cnt);
> >>+	dev_info(&pcidev->dev, "dfl_cnt %d\n", dfl_cnt);
> >
> >dev_dbg() is better?
> 
> I will change to dev_dbg in v2.
> 
> >
> >>+	for (i = 0; i < dfl_cnt; i++) {
> >>+		dfl_res_off = voff + PCI_VNDR_DFLS_RES_OFFSET +
> >>+				      (i * sizeof(dfl_res));
> >>+		pci_read_config_dword(pcidev, dfl_res_off, &dfl_res);
> >>+
> >>+		dev_dbg(&pcidev->dev, "dfl_res 0x%x\n", dfl_res);
> >>+
> >>+		bar = dfl_res & PCI_VND_DFLS_RES_BAR_MASK;
> >
> >FIELD_GET is better?
> 
> I think & will the GENMASK will be better because it will be
> symetrical to the & below for the offset.

Fine.

> 
> >
> >>+
> >>+		if (bar >= PCI_STD_NUM_BARS) {
> >>+			dev_err(&pcidev->dev, "%s bad bar number %d\n",
> >>+				__func__, bar);
> >>+			return -EINVAL;
> >>+		}
> >>+
> >>+		len = pci_resource_len(pcidev, bar);
> >>+
> >>+		if (len == 0) {
> >>+			dev_err(&pcidev->dev, "%s unmapped bar number %d\n",
> >>+				__func__, bar);
> >>+			return -EINVAL;
> >>+		}
> >>+
> >>+		offset = dfl_res & ~PCI_VND_DFLS_RES_BAR_MASK;
> >
> >ditto
> We don't want to use FIELD_GET here because we don't the shifting.

That's correct.

> 
> >
> >>+
> >>+		if (offset >= len) {
> >>+			dev_err(&pcidev->dev, "%s bad offset %u >= %llu\n",
> >>+				__func__, offset, len);
> >>+			return -EINVAL;
> >>+		}
> >>+
> >>+		dev_info(&pcidev->dev, "%s BAR %d offset 0x%x\n", __func__, bar, offset);
> >
> >dev_dbg()?
> 
> I will change to dev_dbg in v2.
> 
> >
> >>+
> >>+		start = pci_resource_start(pcidev, bar) + offset;
> >>+		len -= offset;
> >
> >With these code, I have the following assumption:
> >
> >1. There is only one DFL in one bar, multiple DFLs requires multiple
> >bars.
> >
> >2. The DFL region is from the "offset" to the end of the bar.
> >
> >Are they correct? If yes maybe we should specify them clearly in Doc.
> >
> 
> This code would have the same assumptions as the existing code for finding
> the dfls.  The len value is only used during the walk of the DFL to prevent
> walking too far.  So I think one could have more than one DFL
> on a particular bar as long as the start of the DFLs are different.

OK, I understand.

It is a little different. Previously all the DFL nodes are chained in
one bar. So we have only one DFL in one bar. In no chance we could have
overlapped regions.

Now we can have more DFLs in one bar, so I think it could be better we
know their boundaries earlier.

> 
> >>+
> >>+		if (!PAGE_ALIGNED(start)) {
> >>+			dev_err(&pcidev->dev, "%s unaliged start 0x%llx\n",
> >>+				__func__, start);
> >>+			return -EINVAL;
> >>+		}
> >>+
> >>+		dfl_fpga_enum_info_add_dfl(info, start, len);
> >
> >Do we need some region overlapping check in this func? So we could find
> >the HW problem (e.g. same bar num for multiple DFLs) in early stage.
> >
> 
> I think whatever overlapping check would also need to be in the existing
> code because the logic is the same.

Yes.

Thanks,
Yilun

> 
> >>+	}
> >>+
> >>+	return 0;
> >>+}
> >>+
> >> static int find_dfl_in_bar0(struct pci_dev *pcidev,
> >> 			    struct dfl_fpga_enum_info *info)
> >> {
> >>@@ -221,7 +304,10 @@ static int cci_enumerate_feature_devs(struct pci_dev *pcidev)
> >> 			goto irq_free_exit;
> >> 	}
> >>
> >>-	ret = find_dfl_in_bar0(pcidev, info);
> >>+	ret = find_dfl_in_cfg(pcidev, info);
> >>+
> >>+	if (ret)
> >>+		ret = find_dfl_in_bar0(pcidev, info);
> >
> >The patch is more than the relocation support for DFL. Actually it
> >introduced a different way of DFL finding.
> >
> >Previously it starts at bar0 offset 0, find dfl fme first, then find
> >dfl port according to fme header registers. Now it enumerates every DFL
> >by PCIe VSEC.
> >
> >Maybe we should add more description about the change and why.
> 
> I will highlight this difference in the documentation in v2.
> >
> >Thanks,
> >Yilun
> >
> >>
> >> 	if (ret)
> >> 		goto irq_free_exit;
> >>--
> >>2.25.2
> >
matthew.gerlach@linux.intel.com Nov. 18, 2020, 5:49 p.m. UTC | #12
On Wed, 18 Nov 2020, Wu, Hao wrote:

>> On Tue, 17 Nov 2020, Wu, Hao wrote:
>
> [...]
>
>>>>  Open discussion
>>>>  ===============
>>>> diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c
>>>> index b1b157b41942..5418e8bf2496 100644
>>>> --- a/drivers/fpga/dfl-pci.c
>>>> +++ b/drivers/fpga/dfl-pci.c
>>>> @@ -27,6 +27,13 @@
>>>>  #define DRV_VERSION	"0.8"
>>>>  #define DRV_NAME	"dfl-pci"
>>>>
>>>> +#define PCI_VNDR_ID_DFLS 0x43
>>>
>>> What about PCI_VSEC_ID_INTEL_DFLS?
>>>
>>> Is it possible a different ID chosen by different vendor?
>>
>> I think another vendor could choose their own ID.
>
> If another vendor could choose their own ID, so should we
> check vendor id as well?

Yes, the vendor id should be checked.  I will add it to v2.
>
> [...]
>
>>>> +	for (i = 0; i < dfl_cnt; i++) {
>>>> +		dfl_res_off = voff + PCI_VNDR_DFLS_RES_OFFSET +
>>>> +				      (i * sizeof(dfl_res));
>>>> +		pci_read_config_dword(pcidev, dfl_res_off, &dfl_res);
>>>> +
>>>> +		dev_dbg(&pcidev->dev, "dfl_res 0x%x\n", dfl_res);
>>>> +
>>>> +		bar = dfl_res & PCI_VND_DFLS_RES_BAR_MASK;
>>>> +
>>>> +		if (bar >= PCI_STD_NUM_BARS) {
>>>> +			dev_err(&pcidev->dev, "%s bad bar number %d\n",
>>>> +				__func__, bar);
>>>> +			return -EINVAL;
>>>> +		}
>>>> +
>>>> +		len = pci_resource_len(pcidev, bar);
>>>> +
>>>
>>> Remove this blank line.
>> OK, v2.
>>
>>>
>>>> +		if (len == 0) {
>>>> +			dev_err(&pcidev->dev, "%s unmapped bar
>>>> number %d\n",
>>>
>>> Why "unmapped bar"?
>>
>> How about, "zero length bar"?
>
> I think this checking can be covered by below one, right?
> if (offset >= len)
> ...

I agree. I will make the change in v2.
>
>>
>>>
>>>> +				__func__, bar);
>>>> +			return -EINVAL;
>>>> +		}
>>>> +
>>>> +		offset = dfl_res & ~PCI_VND_DFLS_RES_BAR_MASK;
>>>> +
>>>
>>> Remove this blank line.
>>
>> OK, v2.
>>
>>>
>>>> +		if (offset >= len) {
>>>> +			dev_err(&pcidev->dev, "%s bad offset %u >= %llu\n",
>>>> +				__func__, offset, len);
>>>> +			return -EINVAL;
>>>> +		}
>>>> +
>
> [....]
>
>>>> +
>>>> +		start = pci_resource_start(pcidev, bar) + offset;
>>>> +		len -= offset;
>>>> +
>>>> +		if (!PAGE_ALIGNED(start)) {
>>>
>>> Is this a hard requirement? Or offset should be page aligned per VSEC
>> definition?
>>> Or this is just the requirement from driver point of view. Actually we don't
>> like
>>> to add rules only in driver, so it's better we have this requirement in VSEC
>> definition
>>> with proper documentation.
>>
>> The DFL parsing code ioremaps the memory bounded by start/len.  I thought
>> this would require the start to be page aligned.
>
> If consider mmap the region to userspace, it requires page aligned, but do we
> need to apply this rule for everyone?

I will remove this check in v2.

>
> Thanks
> Hao
>
>
diff mbox series

Patch

diff --git a/Documentation/fpga/dfl.rst b/Documentation/fpga/dfl.rst
index 0404fe6ffc74..c81ceb1e79e2 100644
--- a/Documentation/fpga/dfl.rst
+++ b/Documentation/fpga/dfl.rst
@@ -501,6 +501,16 @@  Developer only needs to provide a sub feature driver with matched feature id.
 FME Partial Reconfiguration Sub Feature driver (see drivers/fpga/dfl-fme-pr.c)
 could be a reference.
 
+Location of DFLs on PCI bus
+===========================
+The start of the DFL is assumed to be offset 0 of bar 0.
+Alternatively, a vendor specific capability structure can be used to
+specify the location of one or more DFLs.  Intel has reserved the
+vendor specific id of 0x43 for this purpose.  The vendor specific
+data begins with a 4 byte count of the number of DFLs followed 4 byte
+Offset/BIR fields for each DFL. Bits 2:0 of Offset/BIR field indicates
+the BAR, and bits 31:3 form the 8 byte aligned offset where bits 2:0 are
+zero.
 
 Open discussion
 ===============
diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c
index b1b157b41942..5418e8bf2496 100644
--- a/drivers/fpga/dfl-pci.c
+++ b/drivers/fpga/dfl-pci.c
@@ -27,6 +27,13 @@ 
 #define DRV_VERSION	"0.8"
 #define DRV_NAME	"dfl-pci"
 
+#define PCI_VNDR_ID_DFLS 0x43
+
+#define PCI_VNDR_DFLS_CNT_OFFSET 8
+#define PCI_VNDR_DFLS_RES_OFFSET 0x0c
+
+#define PCI_VND_DFLS_RES_BAR_MASK 0x7
+
 struct cci_drvdata {
 	struct dfl_fpga_cdev *cdev;	/* container device */
 };
@@ -119,6 +126,82 @@  static int *cci_pci_create_irq_table(struct pci_dev *pcidev, unsigned int nvec)
 	return table;
 }
 
+static int find_dfl_in_cfg(struct pci_dev *pcidev,
+			   struct dfl_fpga_enum_info *info)
+{
+	u32 bar, offset, vndr_hdr, dfl_cnt, dfl_res;
+	int dfl_res_off, i, voff = 0;
+	resource_size_t start, len;
+
+	while ((voff = pci_find_next_ext_capability(pcidev, voff, PCI_EXT_CAP_ID_VNDR))) {
+
+		pci_read_config_dword(pcidev, voff + PCI_VNDR_HEADER, &vndr_hdr);
+
+		dev_dbg(&pcidev->dev,
+			"vendor-specific capability id 0x%x, rev 0x%x len 0x%x\n",
+			PCI_VNDR_HEADER_ID(vndr_hdr),
+			PCI_VNDR_HEADER_REV(vndr_hdr),
+			PCI_VNDR_HEADER_LEN(vndr_hdr));
+
+		if (PCI_VNDR_HEADER_ID(vndr_hdr) == PCI_VNDR_ID_DFLS)
+			break;
+	}
+
+	if (!voff) {
+		dev_dbg(&pcidev->dev, "%s no VSEC found\n", __func__);
+		return -ENODEV;
+	}
+
+	pci_read_config_dword(pcidev, voff + PCI_VNDR_DFLS_CNT_OFFSET, &dfl_cnt);
+	dev_info(&pcidev->dev, "dfl_cnt %d\n", dfl_cnt);
+	for (i = 0; i < dfl_cnt; i++) {
+		dfl_res_off = voff + PCI_VNDR_DFLS_RES_OFFSET +
+				      (i * sizeof(dfl_res));
+		pci_read_config_dword(pcidev, dfl_res_off, &dfl_res);
+
+		dev_dbg(&pcidev->dev, "dfl_res 0x%x\n", dfl_res);
+
+		bar = dfl_res & PCI_VND_DFLS_RES_BAR_MASK;
+
+		if (bar >= PCI_STD_NUM_BARS) {
+			dev_err(&pcidev->dev, "%s bad bar number %d\n",
+				__func__, bar);
+			return -EINVAL;
+		}
+
+		len = pci_resource_len(pcidev, bar);
+
+		if (len == 0) {
+			dev_err(&pcidev->dev, "%s unmapped bar number %d\n",
+				__func__, bar);
+			return -EINVAL;
+		}
+
+		offset = dfl_res & ~PCI_VND_DFLS_RES_BAR_MASK;
+
+		if (offset >= len) {
+			dev_err(&pcidev->dev, "%s bad offset %u >= %llu\n",
+				__func__, offset, len);
+			return -EINVAL;
+		}
+
+		dev_info(&pcidev->dev, "%s BAR %d offset 0x%x\n", __func__, bar, offset);
+
+		start = pci_resource_start(pcidev, bar) + offset;
+		len -= offset;
+
+		if (!PAGE_ALIGNED(start)) {
+			dev_err(&pcidev->dev, "%s unaliged start 0x%llx\n",
+				__func__, start);
+			return -EINVAL;
+		}
+
+		dfl_fpga_enum_info_add_dfl(info, start, len);
+	}
+
+	return 0;
+}
+
 static int find_dfl_in_bar0(struct pci_dev *pcidev,
 			    struct dfl_fpga_enum_info *info)
 {
@@ -221,7 +304,10 @@  static int cci_enumerate_feature_devs(struct pci_dev *pcidev)
 			goto irq_free_exit;
 	}
 
-	ret = find_dfl_in_bar0(pcidev, info);
+	ret = find_dfl_in_cfg(pcidev, info);
+
+	if (ret)
+		ret = find_dfl_in_bar0(pcidev, info);
 
 	if (ret)
 		goto irq_free_exit;