* [PATCH v1 1/7] Documentation: fpga: dfl: add description of IOFS
2022-02-14 11:26 [PATCH v1 0/7] Add Intel OFS support for DFL driver Tianfei zhang
@ 2022-02-14 11:26 ` Tianfei zhang
2022-02-14 12:18 ` Akira Yokosawa
` (2 more replies)
2022-02-14 11:26 ` [PATCH v1 2/7] fpga: dfl: check feature type before parse irq info Tianfei zhang
` (5 subsequent siblings)
6 siblings, 3 replies; 47+ messages in thread
From: Tianfei zhang @ 2022-02-14 11:26 UTC (permalink / raw)
To: hao.wu, trix, mdf, yilun.xu, linux-fpga, linux-doc, linux-kernel
Cc: corbet, Tianfei Zhang
From: Tianfei Zhang <tianfei.zhang@intel.com>
This patch adds description about IOFS support for DFL.
Signed-off-by: Tianfei Zhang <tianfei.zhang@intel.com>
---
Documentation/fpga/dfl.rst | 99 +++++++++++++++++++++++++++++++++++++-
1 file changed, 97 insertions(+), 2 deletions(-)
diff --git a/Documentation/fpga/dfl.rst b/Documentation/fpga/dfl.rst
index ef9eec71f6f3..6f9eae1c1697 100644
--- a/Documentation/fpga/dfl.rst
+++ b/Documentation/fpga/dfl.rst
@@ -58,7 +58,10 @@ interface to FPGA, e.g. the FPGA Management Engine (FME) and Port (more
descriptions on FME and Port in later sections).
Accelerated Function Unit (AFU) represents an FPGA programmable region and
-always connects to a FIU (e.g. a Port) as its child as illustrated above.
+always connects to a FIU (e.g. a Port) as its child as illustrated above, but
+on IOFS design, it introducing Port Gasket which contains AFUs. For DFL perspective,
+the Next_AFU pointer on FIU feature header can point to NULL so the AFU is not
+connects to a FIU(more descriptions on IOFS in later section).
Private Features represent sub features of the FIU and AFU. They could be
various function blocks with different IDs, but all private features which
@@ -134,6 +137,9 @@ reconfigurable region containing an AFU. It controls the communication from SW
to the accelerator and exposes features such as reset and debug. Each FPGA
device may have more than one port, but always one AFU per port.
+On IOFS, it introducing a new hardware unit, Port Gasket, which contains all
+the PR specific modules and regions (more descriptions on IOFS in later section).
+
AFU
===
@@ -143,6 +149,9 @@ used for accelerator-specific control registers.
User-space applications can acquire exclusive access to an AFU attached to a
port by using open() on the port device node and release it using close().
+On IOFS, the AFU is embedded in a Port Gasket. The AFU resource can expose via
+VFs with SRIOV support (more descriptions on IOFS in later section).
+
The following functions are exposed through ioctls:
- Get driver API version (DFL_FPGA_GET_API_VERSION)
@@ -284,7 +293,8 @@ FME is always accessed through the physical function (PF).
Ports (and related AFUs) are accessed via PF by default, but could be exposed
through virtual function (VF) devices via PCIe SRIOV. Each VF only contains
-1 Port and 1 AFU for isolation. Users could assign individual VFs (accelerators)
+1 Port (On IOFS design, the VF is designs without Port) and 1 AFU for isolation.
+Users could assign individual VFs (accelerators)
created via PCIe SRIOV interface, to virtual machines.
The driver organization in virtualization case is illustrated below:
@@ -389,6 +399,91 @@ The device nodes used for ioctl() or mmap() can be referenced through::
/sys/class/fpga_region/<regionX>/<dfl-fme.n>/dev
/sys/class/fpga_region/<regionX>/<dfl-port.n>/dev
+Intel Open FPGA stack
+=====================
+Intel Open FPGA stack aka IOFS, Intel's version of a common core set of
+RTL to allow customers to easily interface to logic and IP on the FPGA.
+IOFS leverage the DFL for the implementation of the FPGA RTL design.
+
+IOFS designs allow for the arrangement of software interfaces across multiple
+PCIe endpoints. Some of these interfaces may be PFs defined in the static region
+that connect to interfaces in an IP that is loaded via Partial Reconfiguration (PR).
+And some of these interfaces may be VFs defined in the PR region that can be
+reconfigured by the end-user. Furthermore, these PFs/VFs may also be arranged
+using a DFL such that features may be discovered and accessed in user space
+(with the aid of a generic kernel driver like vfio-pci). The diagram below depicts
+an example design with two PFs and two VFs. In this example, PF1 implements its
+MMIO space such that it is compatible with the VirtIO framework. The other functions,
+VF0 and VF1, leverage VFIO to export the MMIO space to an application or a hypervisor.
+
+ +-----------------+ +--------------+ +-------------+ +------------+
+ | FPGA Managerment| | VirtIO | | User App | | Virtual |
+ | App | | App | | | | Machine |
+ +--------+--------+ +------+-------+ +------+------+ +-----+------+
+ | | | |
+ | | | |
+ +--------+--------+ +------+-------+ +------+------+ |
+ | DFL Driver | |VirtIO driver | | VFIO | |
+ +--------+--------+ +------+-------+ +------+------+ |
+ | | | |
+ | | | |
+ +--------+--------+ +------+-------+ +------+------+ +----+------+
+ | PF0 | | PF1 | | PF0_VF0 | | PF0_VF1 |
+ +-----------------+ +--------------+ +-------------+ +-----------+
+
+On IOFS, it introducing some enhancements compared with original DFL design.
+1. It introducing Port Gasket in PF0 which is responsible for FPGA management,
+like FME and Port management. The Port Gasket contains all the PR specific modules
+and logic, e.g., PR slot reset/freeze control, user clock, remote STP etc.
+Architecturally, a Port Gasket can have multiple PR slots where user workload can
+be programmed into.
+2. To expend the scalable of FPGA, it can support multiple FPs in static region
+which contain some static functions like VirtIO, diagnostic test, and access over
+VFIO or assigned to VMs easily. Those PFs will not have a Port Unit which without
+PR region (AFU) connected to those PFs, and the end-user cannot partial reconfigurate
+those PFs.
+3. In our previous DFL design, it can only create one VF based in an AFU. To raise
+the efficiency usage of AFU, it can create more than one VFs in an AFU via PCIe
+SRIOV, so those VFs share the PR region and resource.
+
+There is one reference architecture design for IOFS as illustrated below:
+
+ +----------------------+
+ | PF/VF mux/demux |
+ +--+--+-----+------+-+-+
+ | | | | |
+ +------------------------+ | | | |
+ PF0 | +---------+ +-+ | |
+ +---+---+ | +---+----+ | |
+ | DFH | | | DFH | | |
+ +-------+ +-----+----+ +--------+ | |
+ | FME | | VirtIO | | Test | | |
+ +-------+ +----------+ +--------+ | |
+ | Port | PF1 PF2 | |
+ +---+---+ | |
+ | +----------+ |
+ | | ++
+ | | |
+ | | PF0_VF0 | PF0_VF1
+ | +-----------------+-----------+------------+
+ | | +-----+-----------+--------+ |
+ | | | | | | |
+ | | +------+ | +--+ -+ +--+---+ | |
+ | | | CSR | | | DFH | | DFH | | |
+ +-----------+ +------+ | +-----+ +------+ | |
+ | | | DEV | | DEV | | |
+ | | +-----+ +------+ | |
+ | | PR Slot | |
+ | +--------------------------+ |
+ | Port Gasket |
+ +------------------------------------------+
+
+Here are the major changes about DFL structures on IOFS implementation design:
+1. The Port Gasket connects to FIU Port in DFL, but the Next_AFU pointer in
+FIU feature header can point to NULL so that it is no AFU connects to a FIU
+Port.
+2. The VF which include in PR region can start with AFU feature header without
+a FIU Port feature header.
Performance Counters
====================
--
2.17.1
^ permalink raw reply related [flat|nested] 47+ messages in thread* Re: [PATCH v1 1/7] Documentation: fpga: dfl: add description of IOFS
2022-02-14 11:26 ` [PATCH v1 1/7] Documentation: fpga: dfl: add description of IOFS Tianfei zhang
@ 2022-02-14 12:18 ` Akira Yokosawa
2022-02-14 17:56 ` Randy Dunlap
2022-02-15 14:32 ` Tom Rix
2022-02-16 3:34 ` Wu, Hao
2 siblings, 1 reply; 47+ messages in thread
From: Akira Yokosawa @ 2022-02-14 12:18 UTC (permalink / raw)
To: Tianfei zhang
Cc: corbet, hao.wu, linux-doc, linux-fpga, linux-kernel, mdf, trix,
yilun.xu, Akira Yokosawa
Hi,
Just a couple of nits on ReST formatting.
On Mon, 14 Feb 2022 06:26:13 -0500,
Tianfei zhang <tianfei.zhang@intel.com> wrote:
> From: Tianfei Zhang <tianfei.zhang@intel.com>
>
> This patch adds description about IOFS support for DFL.
>
> Signed-off-by: Tianfei Zhang <tianfei.zhang@intel.com>
> ---
> Documentation/fpga/dfl.rst | 99 +++++++++++++++++++++++++++++++++++++-
> 1 file changed, 97 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/fpga/dfl.rst b/Documentation/fpga/dfl.rst
> index ef9eec71f6f3..6f9eae1c1697 100644
> --- a/Documentation/fpga/dfl.rst
> +++ b/Documentation/fpga/dfl.rst
> @@ -58,7 +58,10 @@ interface to FPGA, e.g. the FPGA Management Engine (FME) and Port (more
> descriptions on FME and Port in later sections).
>
> Accelerated Function Unit (AFU) represents an FPGA programmable region and
> -always connects to a FIU (e.g. a Port) as its child as illustrated above.
> +always connects to a FIU (e.g. a Port) as its child as illustrated above, but
> +on IOFS design, it introducing Port Gasket which contains AFUs. For DFL perspective,
> +the Next_AFU pointer on FIU feature header can point to NULL so the AFU is not
> +connects to a FIU(more descriptions on IOFS in later section).
>
> Private Features represent sub features of the FIU and AFU. They could be
> various function blocks with different IDs, but all private features which
> @@ -134,6 +137,9 @@ reconfigurable region containing an AFU. It controls the communication from SW
> to the accelerator and exposes features such as reset and debug. Each FPGA
> device may have more than one port, but always one AFU per port.
>
> +On IOFS, it introducing a new hardware unit, Port Gasket, which contains all
> +the PR specific modules and regions (more descriptions on IOFS in later section).
> +
>
> AFU
> ===
> @@ -143,6 +149,9 @@ used for accelerator-specific control registers.
> User-space applications can acquire exclusive access to an AFU attached to a
> port by using open() on the port device node and release it using close().
>
> +On IOFS, the AFU is embedded in a Port Gasket. The AFU resource can expose via
> +VFs with SRIOV support (more descriptions on IOFS in later section).
> +
> The following functions are exposed through ioctls:
>
> - Get driver API version (DFL_FPGA_GET_API_VERSION)
> @@ -284,7 +293,8 @@ FME is always accessed through the physical function (PF).
>
> Ports (and related AFUs) are accessed via PF by default, but could be exposed
> through virtual function (VF) devices via PCIe SRIOV. Each VF only contains
> -1 Port and 1 AFU for isolation. Users could assign individual VFs (accelerators)
> +1 Port (On IOFS design, the VF is designs without Port) and 1 AFU for isolation.
> +Users could assign individual VFs (accelerators)
> created via PCIe SRIOV interface, to virtual machines.
>
> The driver organization in virtualization case is illustrated below:
> @@ -389,6 +399,91 @@ The device nodes used for ioctl() or mmap() can be referenced through::
> /sys/class/fpga_region/<regionX>/<dfl-fme.n>/dev
> /sys/class/fpga_region/<regionX>/<dfl-port.n>/dev
>
> +Intel Open FPGA stack
> +=====================
Here you need an empty line.
> +Intel Open FPGA stack aka IOFS, Intel's version of a common core set of
> +RTL to allow customers to easily interface to logic and IP on the FPGA.
> +IOFS leverage the DFL for the implementation of the FPGA RTL design.
> +
> +IOFS designs allow for the arrangement of software interfaces across multiple
> +PCIe endpoints. Some of these interfaces may be PFs defined in the static region
> +that connect to interfaces in an IP that is loaded via Partial Reconfiguration (PR).
> +And some of these interfaces may be VFs defined in the PR region that can be
> +reconfigured by the end-user. Furthermore, these PFs/VFs may also be arranged
> +using a DFL such that features may be discovered and accessed in user space
> +(with the aid of a generic kernel driver like vfio-pci). The diagram below depicts
> +an example design with two PFs and two VFs. In this example, PF1 implements its
> +MMIO space such that it is compatible with the VirtIO framework. The other functions,
> +VF0 and VF1, leverage VFIO to export the MMIO space to an application or a hypervisor.
> +
For an ASCII-art to be recognized as such, you need a leading "::" here.
::
> + +-----------------+ +--------------+ +-------------+ +------------+
> + | FPGA Managerment| | VirtIO | | User App | | Virtual |
> + | App | | App | | | | Machine |
> + +--------+--------+ +------+-------+ +------+------+ +-----+------+
> + | | | |
> + | | | |
> + +--------+--------+ +------+-------+ +------+------+ |
> + | DFL Driver | |VirtIO driver | | VFIO | |
> + +--------+--------+ +------+-------+ +------+------+ |
> + | | | |
> + | | | |
> + +--------+--------+ +------+-------+ +------+------+ +----+------+
> + | PF0 | | PF1 | | PF0_VF0 | | PF0_VF1 |
> + +-----------------+ +--------------+ +-------------+ +-----------+
> +
> +On IOFS, it introducing some enhancements compared with original DFL design.
Again, you need an empty line here.
> +1. It introducing Port Gasket in PF0 which is responsible for FPGA management,
> +like FME and Port management. The Port Gasket contains all the PR specific modules
> +and logic, e.g., PR slot reset/freeze control, user clock, remote STP etc.
> +Architecturally, a Port Gasket can have multiple PR slots where user workload can
> +be programmed into.
> +2. To expend the scalable of FPGA, it can support multiple FPs in static region
> +which contain some static functions like VirtIO, diagnostic test, and access over
> +VFIO or assigned to VMs easily. Those PFs will not have a Port Unit which without
> +PR region (AFU) connected to those PFs, and the end-user cannot partial reconfigurate
> +those PFs.
> +3. In our previous DFL design, it can only create one VF based in an AFU. To raise
> +the efficiency usage of AFU, it can create more than one VFs in an AFU via PCIe
> +SRIOV, so those VFs share the PR region and resource.
This enumeration list needs some indent tweaks.
1. It introducing Port Gasket in PF0 which is responsible for FPGA management,
like FME and Port management. The Port Gasket contains all the PR specific modules
and logic, e.g., PR slot reset/freeze control, user clock, remote STP etc.
Architecturally, a Port Gasket can have multiple PR slots where user workload can
be programmed into.
2. To expend the scalable of FPGA, it can support multiple FPs in static region
which contain some static functions like VirtIO, diagnostic test, and access over
VFIO or assigned to VMs easily. Those PFs will not have a Port Unit which without
PR region (AFU) connected to those PFs, and the end-user cannot partial reconfigurate
those PFs.
3. In our previous DFL design, it can only create one VF based in an AFU. To raise
the efficiency usage of AFU, it can create more than one VFs in an AFU via PCIe
SRIOV, so those VFs share the PR region and resource.
> +
> +There is one reference architecture design for IOFS as illustrated below:
::
(This "::" can also lead an ASCII-art.)
> +
> + +----------------------+
> + | PF/VF mux/demux |
> + +--+--+-----+------+-+-+
> + | | | | |
> + +------------------------+ | | | |
> + PF0 | +---------+ +-+ | |
> + +---+---+ | +---+----+ | |
> + | DFH | | | DFH | | |
> + +-------+ +-----+----+ +--------+ | |
> + | FME | | VirtIO | | Test | | |
> + +-------+ +----------+ +--------+ | |
> + | Port | PF1 PF2 | |
> + +---+---+ | |
> + | +----------+ |
> + | | ++
> + | | |
> + | | PF0_VF0 | PF0_VF1
> + | +-----------------+-----------+------------+
> + | | +-----+-----------+--------+ |
> + | | | | | | |
> + | | +------+ | +--+ -+ +--+---+ | |
> + | | | CSR | | | DFH | | DFH | | |
> + +-----------+ +------+ | +-----+ +------+ | |
> + | | | DEV | | DEV | | |
> + | | +-----+ +------+ | |
> + | | PR Slot | |
> + | +--------------------------+ |
> + | Port Gasket |
> + +------------------------------------------+
> +
> +Here are the major changes about DFL structures on IOFS implementation design:
Again, you need an empty line.
> +1. The Port Gasket connects to FIU Port in DFL, but the Next_AFU pointer in
> +FIU feature header can point to NULL so that it is no AFU connects to a FIU
> +Port.
> +2. The VF which include in PR region can start with AFU feature header without
> +a FIU Port feature header.
As above, this list should look:
1. The Port Gasket connects to FIU Port in DFL, but the Next_AFU pointer in
FIU feature header can point to NULL so that it is no AFU connects to a FIU
Port.
2. The VF which include in PR region can start with AFU feature header without
a FIU Port feature header.
>
> Performance Counters
> ====================
> --
> 2.17.1
I'd recommend running "make htmdocs" and see if the pages are rendered as you
expect.
Thanks, Akira
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH v1 1/7] Documentation: fpga: dfl: add description of IOFS
2022-02-14 12:18 ` Akira Yokosawa
@ 2022-02-14 17:56 ` Randy Dunlap
2022-02-14 23:27 ` Zhang, Tianfei
0 siblings, 1 reply; 47+ messages in thread
From: Randy Dunlap @ 2022-02-14 17:56 UTC (permalink / raw)
To: Akira Yokosawa, Tianfei zhang
Cc: corbet, hao.wu, linux-doc, linux-fpga, linux-kernel, mdf, trix, yilun.xu
On 2/14/22 04:18, Akira Yokosawa wrote:
> Hi,
>
> Just a couple of nits on ReST formatting.
>
Thanks :)
>
> I'd recommend running "make htmdocs" and see if the pages are rendered as you
> expect.
I think that's "make htmldocs". But please do use it.
--
~Randy
^ permalink raw reply [flat|nested] 47+ messages in thread
* RE: [PATCH v1 1/7] Documentation: fpga: dfl: add description of IOFS
2022-02-14 17:56 ` Randy Dunlap
@ 2022-02-14 23:27 ` Zhang, Tianfei
0 siblings, 0 replies; 47+ messages in thread
From: Zhang, Tianfei @ 2022-02-14 23:27 UTC (permalink / raw)
To: Randy Dunlap, Akira Yokosawa
Cc: corbet, Wu, Hao, linux-doc, linux-fpga, linux-kernel, mdf, trix,
Xu, Yilun
> -----Original Message-----
> From: Randy Dunlap <rdunlap@infradead.org>
> Sent: Tuesday, February 15, 2022 1:57 AM
> To: Akira Yokosawa <akiyks@gmail.com>; Zhang, Tianfei
> <tianfei.zhang@intel.com>
> Cc: corbet@lwn.net; Wu, Hao <hao.wu@intel.com>; linux-
> doc@vger.kernel.org; linux-fpga@vger.kernel.org; linux-
> kernel@vger.kernel.org; mdf@kernel.org; trix@redhat.com; Xu, Yilun
> <yilun.xu@intel.com>
> Subject: Re: [PATCH v1 1/7] Documentation: fpga: dfl: add description of IOFS
>
>
>
> On 2/14/22 04:18, Akira Yokosawa wrote:
> > Hi,
> >
> > Just a couple of nits on ReST formatting.
> >
>
> Thanks :)
>
>
> >
> > I'd recommend running "make htmdocs" and see if the pages are rendered
> > as you expect.
>
> I think that's "make htmldocs". But please do use it.
Thanks，I will fix it on next version.
>
> --
> ~Randy
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v1 1/7] Documentation: fpga: dfl: add description of IOFS
2022-02-14 11:26 ` [PATCH v1 1/7] Documentation: fpga: dfl: add description of IOFS Tianfei zhang
2022-02-14 12:18 ` Akira Yokosawa
@ 2022-02-15 14:32 ` Tom Rix
2022-02-16 3:34 ` Wu, Hao
2 siblings, 0 replies; 47+ messages in thread
From: Tom Rix @ 2022-02-15 14:32 UTC (permalink / raw)
To: Tianfei zhang, hao.wu, mdf, yilun.xu, linux-fpga, linux-doc,
linux-kernel
Cc: corbet
On 2/14/22 3:26 AM, Tianfei zhang wrote:
> From: Tianfei Zhang <tianfei.zhang@intel.com>
>
> This patch adds description about IOFS support for DFL.
>
This needs work.
Several times 'Port Gasket' was mentioned but there is no section
describing what it is.
It is not clear if iofs supersedes dfl or extends it.
It is not clear how a driver probes to tell the difference between the two.
This document should set up why the other changes in the patchset are
needed.
Add yourself to authors list.
Tom
> Signed-off-by: Tianfei Zhang <tianfei.zhang@intel.com>
> ---
> Documentation/fpga/dfl.rst | 99 +++++++++++++++++++++++++++++++++++++-
> 1 file changed, 97 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/fpga/dfl.rst b/Documentation/fpga/dfl.rst
> index ef9eec71f6f3..6f9eae1c1697 100644
> --- a/Documentation/fpga/dfl.rst
> +++ b/Documentation/fpga/dfl.rst
> @@ -58,7 +58,10 @@ interface to FPGA, e.g. the FPGA Management Engine (FME) and Port (more
> descriptions on FME and Port in later sections).
>
> Accelerated Function Unit (AFU) represents an FPGA programmable region and
> -always connects to a FIU (e.g. a Port) as its child as illustrated above.
> +always connects to a FIU (e.g. a Port) as its child as illustrated above, but
> +on IOFS design, it introducing Port Gasket which contains AFUs. For DFL perspective,
> +the Next_AFU pointer on FIU feature header can point to NULL so the AFU is not
> +connects to a FIU(more descriptions on IOFS in later section).
>
> Private Features represent sub features of the FIU and AFU. They could be
> various function blocks with different IDs, but all private features which
> @@ -134,6 +137,9 @@ reconfigurable region containing an AFU. It controls the communication from SW
> to the accelerator and exposes features such as reset and debug. Each FPGA
> device may have more than one port, but always one AFU per port.
>
> +On IOFS, it introducing a new hardware unit, Port Gasket, which contains all
> +the PR specific modules and regions (more descriptions on IOFS in later section).
> +
>
> AFU
> ===
> @@ -143,6 +149,9 @@ used for accelerator-specific control registers.
> User-space applications can acquire exclusive access to an AFU attached to a
> port by using open() on the port device node and release it using close().
>
> +On IOFS, the AFU is embedded in a Port Gasket. The AFU resource can expose via
> +VFs with SRIOV support (more descriptions on IOFS in later section).
> +
> The following functions are exposed through ioctls:
>
> - Get driver API version (DFL_FPGA_GET_API_VERSION)
> @@ -284,7 +293,8 @@ FME is always accessed through the physical function (PF).
>
> Ports (and related AFUs) are accessed via PF by default, but could be exposed
> through virtual function (VF) devices via PCIe SRIOV. Each VF only contains
> -1 Port and 1 AFU for isolation. Users could assign individual VFs (accelerators)
> +1 Port (On IOFS design, the VF is designs without Port) and 1 AFU for isolation.
> +Users could assign individual VFs (accelerators)
> created via PCIe SRIOV interface, to virtual machines.
>
> The driver organization in virtualization case is illustrated below:
> @@ -389,6 +399,91 @@ The device nodes used for ioctl() or mmap() can be referenced through::
> /sys/class/fpga_region/<regionX>/<dfl-fme.n>/dev
> /sys/class/fpga_region/<regionX>/<dfl-port.n>/dev
>
> +Intel Open FPGA stack
> +=====================
> +Intel Open FPGA stack aka IOFS, Intel's version of a common core set of
> +RTL to allow customers to easily interface to logic and IP on the FPGA.
> +IOFS leverage the DFL for the implementation of the FPGA RTL design.
> +
> +IOFS designs allow for the arrangement of software interfaces across multiple
> +PCIe endpoints. Some of these interfaces may be PFs defined in the static region
> +that connect to interfaces in an IP that is loaded via Partial Reconfiguration (PR).
> +And some of these interfaces may be VFs defined in the PR region that can be
> +reconfigured by the end-user. Furthermore, these PFs/VFs may also be arranged
> +using a DFL such that features may be discovered and accessed in user space
> +(with the aid of a generic kernel driver like vfio-pci). The diagram below depicts
> +an example design with two PFs and two VFs. In this example, PF1 implements its
> +MMIO space such that it is compatible with the VirtIO framework. The other functions,
> +VF0 and VF1, leverage VFIO to export the MMIO space to an application or a hypervisor.
> +
> + +-----------------+ +--------------+ +-------------+ +------------+
> + | FPGA Managerment| | VirtIO | | User App | | Virtual |
> + | App | | App | | | | Machine |
> + +--------+--------+ +------+-------+ +------+------+ +-----+------+
> + | | | |
> + | | | |
> + +--------+--------+ +------+-------+ +------+------+ |
> + | DFL Driver | |VirtIO driver | | VFIO | |
> + +--------+--------+ +------+-------+ +------+------+ |
> + | | | |
> + | | | |
> + +--------+--------+ +------+-------+ +------+------+ +----+------+
> + | PF0 | | PF1 | | PF0_VF0 | | PF0_VF1 |
> + +-----------------+ +--------------+ +-------------+ +-----------+
> +
> +On IOFS, it introducing some enhancements compared with original DFL design.
> +1. It introducing Port Gasket in PF0 which is responsible for FPGA management,
> +like FME and Port management. The Port Gasket contains all the PR specific modules
> +and logic, e.g., PR slot reset/freeze control, user clock, remote STP etc.
> +Architecturally, a Port Gasket can have multiple PR slots where user workload can
> +be programmed into.
> +2. To expend the scalable of FPGA, it can support multiple FPs in static region
> +which contain some static functions like VirtIO, diagnostic test, and access over
> +VFIO or assigned to VMs easily. Those PFs will not have a Port Unit which without
> +PR region (AFU) connected to those PFs, and the end-user cannot partial reconfigurate
> +those PFs.
> +3. In our previous DFL design, it can only create one VF based in an AFU. To raise
> +the efficiency usage of AFU, it can create more than one VFs in an AFU via PCIe
> +SRIOV, so those VFs share the PR region and resource.
> +
> +There is one reference architecture design for IOFS as illustrated below:
> +
> + +----------------------+
> + | PF/VF mux/demux |
> + +--+--+-----+------+-+-+
> + | | | | |
> + +------------------------+ | | | |
> + PF0 | +---------+ +-+ | |
> + +---+---+ | +---+----+ | |
> + | DFH | | | DFH | | |
> + +-------+ +-----+----+ +--------+ | |
> + | FME | | VirtIO | | Test | | |
> + +-------+ +----------+ +--------+ | |
> + | Port | PF1 PF2 | |
> + +---+---+ | |
> + | +----------+ |
> + | | ++
> + | | |
> + | | PF0_VF0 | PF0_VF1
> + | +-----------------+-----------+------------+
> + | | +-----+-----------+--------+ |
> + | | | | | | |
> + | | +------+ | +--+ -+ +--+---+ | |
> + | | | CSR | | | DFH | | DFH | | |
> + +-----------+ +------+ | +-----+ +------+ | |
> + | | | DEV | | DEV | | |
> + | | +-----+ +------+ | |
> + | | PR Slot | |
> + | +--------------------------+ |
> + | Port Gasket |
> + +------------------------------------------+
> +
> +Here are the major changes about DFL structures on IOFS implementation design:
> +1. The Port Gasket connects to FIU Port in DFL, but the Next_AFU pointer in
> +FIU feature header can point to NULL so that it is no AFU connects to a FIU
> +Port.
> +2. The VF which include in PR region can start with AFU feature header without
> +a FIU Port feature header.
>
> Performance Counters
> ====================
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v1 1/7] Documentation: fpga: dfl: add description of IOFS
2022-02-14 11:26 ` [PATCH v1 1/7] Documentation: fpga: dfl: add description of IOFS Tianfei zhang
2022-02-14 12:18 ` Akira Yokosawa
2022-02-15 14:32 ` Tom Rix
@ 2022-02-16 3:34 ` Wu, Hao
2022-02-21 7:39 ` Zhang, Tianfei
2 siblings, 1 reply; 47+ messages in thread
From: Wu, Hao @ 2022-02-16 3:34 UTC (permalink / raw)
To: Zhang, Tianfei, trix, mdf, Xu, Yilun, linux-fpga, linux-doc,
linux-kernel
Cc: corbet
> Subject: [PATCH v1 1/7] Documentation: fpga: dfl: add description of IOFS
>
> From: Tianfei Zhang <tianfei.zhang@intel.com>
>
> This patch adds description about IOFS support for DFL.
>
> Signed-off-by: Tianfei Zhang <tianfei.zhang@intel.com>
> ---
> Documentation/fpga/dfl.rst | 99 +++++++++++++++++++++++++++++++++++++-
> 1 file changed, 97 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/fpga/dfl.rst b/Documentation/fpga/dfl.rst
> index ef9eec71f6f3..6f9eae1c1697 100644
> --- a/Documentation/fpga/dfl.rst
> +++ b/Documentation/fpga/dfl.rst
> @@ -58,7 +58,10 @@ interface to FPGA, e.g. the FPGA Management Engine
> (FME) and Port (more
> descriptions on FME and Port in later sections).
>
> Accelerated Function Unit (AFU) represents an FPGA programmable region and
> -always connects to a FIU (e.g. a Port) as its child as illustrated above.
> +always connects to a FIU (e.g. a Port) as its child as illustrated above, but
> +on IOFS design, it introducing Port Gasket which contains AFUs. For DFL
> perspective,
> +the Next_AFU pointer on FIU feature header can point to NULL so the AFU is
> not
> +connects to a FIU(more descriptions on IOFS in later section).
>
> Private Features represent sub features of the FIU and AFU. They could be
> various function blocks with different IDs, but all private features which
> @@ -134,6 +137,9 @@ reconfigurable region containing an AFU. It controls the
> communication from SW
> to the accelerator and exposes features such as reset and debug. Each FPGA
> device may have more than one port, but always one AFU per port.
>
> +On IOFS, it introducing a new hardware unit, Port Gasket, which contains all
> +the PR specific modules and regions (more descriptions on IOFS in later section).
What's the different between the PORT we have now for DFH, and the new one
in IOFS?
> +
>
> AFU
> ===
> @@ -143,6 +149,9 @@ used for accelerator-specific control registers.
> User-space applications can acquire exclusive access to an AFU attached to a
> port by using open() on the port device node and release it using close().
>
> +On IOFS, the AFU is embedded in a Port Gasket. The AFU resource can expose
> via
> +VFs with SRIOV support (more descriptions on IOFS in later section).
> +
> The following functions are exposed through ioctls:
>
> - Get driver API version (DFL_FPGA_GET_API_VERSION)
> @@ -284,7 +293,8 @@ FME is always accessed through the physical function
> (PF).
>
> Ports (and related AFUs) are accessed via PF by default, but could be exposed
> through virtual function (VF) devices via PCIe SRIOV. Each VF only contains
> -1 Port and 1 AFU for isolation. Users could assign individual VFs (accelerators)
> +1 Port (On IOFS design, the VF is designs without Port) and 1 AFU for isolation.
> +Users could assign individual VFs (accelerators)
> created via PCIe SRIOV interface, to virtual machines.
>
> The driver organization in virtualization case is illustrated below:
> @@ -389,6 +399,91 @@ The device nodes used for ioctl() or mmap() can be
> referenced through::
> /sys/class/fpga_region/<regionX>/<dfl-fme.n>/dev
> /sys/class/fpga_region/<regionX>/<dfl-port.n>/dev
>
> +Intel Open FPGA stack
> +=====================
> +Intel Open FPGA stack aka IOFS, Intel's version of a common core set of
> +RTL to allow customers to easily interface to logic and IP on the FPGA.
> +IOFS leverage the DFL for the implementation of the FPGA RTL design.
> +
> +IOFS designs allow for the arrangement of software interfaces across multiple
> +PCIe endpoints. Some of these interfaces may be PFs defined in the static
> region
> +that connect to interfaces in an IP that is loaded via Partial Reconfiguration
> (PR).
> +And some of these interfaces may be VFs defined in the PR region that can be
> +reconfigured by the end-user. Furthermore, these PFs/VFs may also be
> arranged
> +using a DFL such that features may be discovered and accessed in user space
> +(with the aid of a generic kernel driver like vfio-pci). The diagram below depicts
> +an example design with two PFs and two VFs. In this example, PF1 implements
> its
> +MMIO space such that it is compatible with the VirtIO framework. The other
> functions,
> +VF0 and VF1, leverage VFIO to export the MMIO space to an application or a
> hypervisor.
So PORT will never be exposed to VM in IOFS design, is my understanding correct?
> +
> + +-----------------+ +--------------+ +-------------+ +------------+
> + | FPGA Managerment| | VirtIO | | User App | | Virtual |
s/Managerment/Management/
> + | App | | App | | | | Machine |
> + +--------+--------+ +------+-------+ +------+------+ +-----+------+
> + | | | |
> + | | | |
> + +--------+--------+ +------+-------+ +------+------+ |
> + | DFL Driver | |VirtIO driver | | VFIO | |
> + +--------+--------+ +------+-------+ +------+------+ |
> + | | | |
> + | | | |
> + +--------+--------+ +------+-------+ +------+------+ +----+------+
> + | PF0 | | PF1 | | PF0_VF0 | | PF0_VF1 |
> + +-----------------+ +--------------+ +-------------+ +-----------+
> +
> +On IOFS, it introducing some enhancements compared with original DFL design.
> +1. It introducing Port Gasket in PF0 which is responsible for FPGA management,
> +like FME and Port management. The Port Gasket contains all the PR specific
> modules
So in IOFS, in PF0, we always have FME and PORT DFH, is my understanding correct?
Then why we need patch #3?
Another question is in IOFS, do we need to support multiple PR regions/Ports?
If that is the case, how should we know which VFs belongs to PORT1 or PORT2?
> +and logic, e.g., PR slot reset/freeze control, user clock, remote STP etc.
> +Architecturally, a Port Gasket can have multiple PR slots where user workload
> can
> +be programmed into.
> +2. To expend the scalable of FPGA, it can support multiple FPs in static region
s/FPs/PFs/
> +which contain some static functions like VirtIO, diagnostic test, and access over
> +VFIO or assigned to VMs easily. Those PFs will not have a Port Unit which
> without
> +PR region (AFU) connected to those PFs, and the end-user cannot partial
> reconfigurate
s/reconfigurate/reconfigure/
> +those PFs.
> +3. In our previous DFL design, it can only create one VF based in an AFU. To
> raise
> +the efficiency usage of AFU, it can create more than one VFs in an AFU via PCIe
> +SRIOV, so those VFs share the PR region and resource.
> +
> +There is one reference architecture design for IOFS as illustrated below:
> +
> + +----------------------+
> + | PF/VF mux/demux |
> + +--+--+-----+------+-+-+
> + | | | | |
> + +------------------------+ | | | |
> + PF0 | +---------+ +-+ | |
> + +---+---+ | +---+----+ | |
> + | DFH | | | DFH | | |
> + +-------+ +-----+----+ +--------+ | |
> + | FME | | VirtIO | | Test | | |
> + +-------+ +----------+ +--------+ | |
> + | Port | PF1 PF2 | |
> + +---+---+ | |
> + | +----------+ |
> + | | ++
> + | | |
> + | | PF0_VF0 | PF0_VF1
> + | +-----------------+-----------+------------+
> + | | +-----+-----------+--------+ |
> + | | | | | | |
> + | | +------+ | +--+ -+ +--+---+ | |
> + | | | CSR | | | DFH | | DFH | | |
> + +-----------+ +------+ | +-----+ +------+ | |
> + | | | DEV | | DEV | | |
> + | | +-----+ +------+ | |
> + | | PR Slot | |
> + | +--------------------------+ |
> + | Port Gasket |
> + +------------------------------------------+
> +
> +Here are the major changes about DFL structures on IOFS implementation
> design:
> +1. The Port Gasket connects to FIU Port in DFL, but the Next_AFU pointer in
> +FIU feature header can point to NULL so that it is no AFU connects to a FIU
> +Port.
> +2. The VF which include in PR region can start with AFU feature header without
> +a FIU Port feature header.
What about PF2 in static region? Which type of DFH will be used?
Thanks
Hao
>
> Performance Counters
> ====================
> --
> 2.17.1
^ permalink raw reply [flat|nested] 47+ messages in thread
* RE: [PATCH v1 1/7] Documentation: fpga: dfl: add description of IOFS
2022-02-16 3:34 ` Wu, Hao
@ 2022-02-21 7:39 ` Zhang, Tianfei
0 siblings, 0 replies; 47+ messages in thread
From: Zhang, Tianfei @ 2022-02-21 7:39 UTC (permalink / raw)
To: Wu, Hao, trix, mdf, Xu, Yilun, linux-fpga, linux-doc, linux-kernel; +Cc: corbet
> -----Original Message-----
> From: Wu, Hao <hao.wu@intel.com>
> Sent: Wednesday, February 16, 2022 11:35 AM
> To: Zhang, Tianfei <tianfei.zhang@intel.com>; trix@redhat.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
> Subject: Re: [PATCH v1 1/7] Documentation: fpga: dfl: add description of IOFS
>
> > Subject: [PATCH v1 1/7] Documentation: fpga: dfl: add description of
> > IOFS
> >
> > From: Tianfei Zhang <tianfei.zhang@intel.com>
> >
> > This patch adds description about IOFS support for DFL.
> >
> > Signed-off-by: Tianfei Zhang <tianfei.zhang@intel.com>
> > ---
> > Documentation/fpga/dfl.rst | 99
> > +++++++++++++++++++++++++++++++++++++-
> > 1 file changed, 97 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/fpga/dfl.rst b/Documentation/fpga/dfl.rst
> > index ef9eec71f6f3..6f9eae1c1697 100644
> > --- a/Documentation/fpga/dfl.rst
> > +++ b/Documentation/fpga/dfl.rst
> > @@ -58,7 +58,10 @@ interface to FPGA, e.g. the FPGA Management Engine
> > (FME) and Port (more
> > descriptions on FME and Port in later sections).
> >
> > Accelerated Function Unit (AFU) represents an FPGA programmable
> > region and -always connects to a FIU (e.g. a Port) as its child as illustrated
> above.
> > +always connects to a FIU (e.g. a Port) as its child as illustrated
> > +above, but on IOFS design, it introducing Port Gasket which contains
> > +AFUs. For DFL
> > perspective,
> > +the Next_AFU pointer on FIU feature header can point to NULL so the
> > +AFU is
> > not
> > +connects to a FIU(more descriptions on IOFS in later section).
> >
> > Private Features represent sub features of the FIU and AFU. They
> > could be various function blocks with different IDs, but all private
> > features which @@ -134,6 +137,9 @@ reconfigurable region containing an
> > AFU. It controls the communication from SW to the accelerator and
> > exposes features such as reset and debug. Each FPGA device may have
> > more than one port, but always one AFU per port.
> >
> > +On IOFS, it introducing a new hardware unit, Port Gasket, which
> > +contains all the PR specific modules and regions (more descriptions on IOFS in
> later section).
>
> What's the different between the PORT we have now for DFH, and the new one
> in IOFS?
From DFL perspective , the PORT concept is identical between IOFS and old card like N3000 card.
The major different is that the IOFS introducing a new hardware concept: Port Gasket, which include the PR slot,
Port control, Port user clock control and Port errors.
>
> > +
> >
> > AFU
> > ===
> > @@ -143,6 +149,9 @@ used for accelerator-specific control registers.
> > User-space applications can acquire exclusive access to an AFU
> > attached to a port by using open() on the port device node and release it using
> close().
> >
> > +On IOFS, the AFU is embedded in a Port Gasket. The AFU resource can
> > +expose
> > via
> > +VFs with SRIOV support (more descriptions on IOFS in later section).
> > +
> > The following functions are exposed through ioctls:
> >
> > - Get driver API version (DFL_FPGA_GET_API_VERSION) @@ -284,7 +293,8
> > @@ FME is always accessed through the physical function (PF).
> >
> > Ports (and related AFUs) are accessed via PF by default, but could be
> > exposed through virtual function (VF) devices via PCIe SRIOV. Each VF
> > only contains
> > -1 Port and 1 AFU for isolation. Users could assign individual VFs
> > (accelerators)
> > +1 Port (On IOFS design, the VF is designs without Port) and 1 AFU for
> isolation.
> > +Users could assign individual VFs (accelerators)
> > created via PCIe SRIOV interface, to virtual machines.
> >
> > The driver organization in virtualization case is illustrated below:
> > @@ -389,6 +399,91 @@ The device nodes used for ioctl() or mmap() can
> > be referenced through::
> > /sys/class/fpga_region/<regionX>/<dfl-fme.n>/dev
> > /sys/class/fpga_region/<regionX>/<dfl-port.n>/dev
> >
> > +Intel Open FPGA stack
> > +=====================
> > +Intel Open FPGA stack aka IOFS, Intel's version of a common core set
> > +of RTL to allow customers to easily interface to logic and IP on the FPGA.
> > +IOFS leverage the DFL for the implementation of the FPGA RTL design.
> > +
> > +IOFS designs allow for the arrangement of software interfaces across
> > +multiple PCIe endpoints. Some of these interfaces may be PFs defined
> > +in the static
> > region
> > +that connect to interfaces in an IP that is loaded via Partial
> > +Reconfiguration
> > (PR).
> > +And some of these interfaces may be VFs defined in the PR region that
> > +can be reconfigured by the end-user. Furthermore, these PFs/VFs may
> > +also be
> > arranged
> > +using a DFL such that features may be discovered and accessed in user
> > +space (with the aid of a generic kernel driver like vfio-pci). The
> > +diagram below depicts an example design with two PFs and two VFs. In
> > +this example, PF1 implements
> > its
> > +MMIO space such that it is compatible with the VirtIO framework. The
> > +other
> > functions,
> > +VF0 and VF1, leverage VFIO to export the MMIO space to an application
> > +or a
> > hypervisor.
>
> So PORT will never be exposed to VM in IOFS design, is my understanding
> correct?
On IOFS, there are many methods to access the AFU for virtualization and non-virtualization usage.
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.
In this model, the follow of exposing to VM is the same we have in N3000 and N5000 card.
The major follow is:
* release the port device.
* configure the SRIOV
* assign the VF to VM
* put the port device back to PF after finished access in VM
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, each PR slot has a dedicate Port device, the follow of exposing to VM is the same we have in N3000 and N5000 card.
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/PR slot, so we don't need to release the Port device.
before we assign the VF to VM.
Which model will deploy in IOFS product depends on the RTL developer.
>
> > +
> > + +-----------------+ +--------------+ +-------------+ +------------+
> > + | FPGA Managerment| | VirtIO | | User App | | Virtual |
>
> s/Managerment/Management/
Thanks, I will fix it on next version.
>
> > + | App | | App | | | | Machine |
> > + +--------+--------+ +------+-------+ +------+------+ +-----+------+
> > + | | | |
> > + | | | |
> > + +--------+--------+ +------+-------+ +------+------+ |
> > + | DFL Driver | |VirtIO driver | | VFIO | |
> > + +--------+--------+ +------+-------+ +------+------+ |
> > + | | | |
> > + | | | |
> > + +--------+--------+ +------+-------+ +------+------+ +----+------+
> > + | PF0 | | PF1 | | PF0_VF0 | | PF0_VF1 |
> > + +-----------------+ +--------------+ +-------------+ +-----------+
> > +
> > +On IOFS, it introducing some enhancements compared with original DFL
> design.
> > +1. It introducing Port Gasket in PF0 which is responsible for FPGA
> > +management, like FME and Port management. The Port Gasket contains
> > +all the PR specific
> > modules
>
> So in IOFS, in PF0, we always have FME and PORT DFH, is my understanding
> correct?
Yes.
> Then why we need patch #3?
This is for " Multiple VFs per PR slot" model, in this model, the Port device would not connected to AFU (the BarID of Port device should be set to invalid),
so we just can access PR slot/AFU resource via VFs.
>
> Another question is in IOFS, do we need to support multiple PR regions/Ports?
> If that is the case, how should we know which VFs belongs to PORT1 or PORT2?
>
> > +and logic, e.g., PR slot reset/freeze control, user clock, remote STP etc.
> > +Architecturally, a Port Gasket can have multiple PR slots where user
> > +workload
> > can
> > +be programmed into.
> > +2. To expend the scalable of FPGA, it can support multiple FPs in
> > +static region
>
> s/FPs/PFs/
I will fix on next version.
>
> > +which contain some static functions like VirtIO, diagnostic test, and
> > +access over VFIO or assigned to VMs easily. Those PFs will not have a
> > +Port Unit which
> > without
> > +PR region (AFU) connected to those PFs, and the end-user cannot
> > +partial
> > reconfigurate
>
> s/reconfigurate/reconfigure/
I will fix on next version.
>
> > +those PFs.
> > +3. In our previous DFL design, it can only create one VF based in an
> > +AFU. To
> > raise
> > +the efficiency usage of AFU, it can create more than one VFs in an
> > +AFU via PCIe SRIOV, so those VFs share the PR region and resource.
> > +
> > +There is one reference architecture design for IOFS as illustrated below:
> > +
> > + +----------------------+
> > + | PF/VF mux/demux |
> > + +--+--+-----+------+-+-+
> > + | | | | |
> > + +------------------------+ | | | |
> > + PF0 | +---------+ +-+ | |
> > + +---+---+ | +---+----+ | |
> > + | DFH | | | DFH | | |
> > + +-------+ +-----+----+ +--------+ | |
> > + | FME | | VirtIO | | Test | | |
> > + +-------+ +----------+ +--------+ | |
> > + | Port | PF1 PF2 | |
> > + +---+---+ | |
> > + | +----------+ |
> > + | | ++
> > + | | |
> > + | | PF0_VF0 | PF0_VF1
> > + | +-----------------+-----------+------------+
> > + | | +-----+-----------+--------+ |
> > + | | | | | | |
> > + | | +------+ | +--+ -+ +--+---+ | |
> > + | | | CSR | | | DFH | | DFH | | |
> > + +-----------+ +------+ | +-----+ +------+ | |
> > + | | | DEV | | DEV | | |
> > + | | +-----+ +------+ | |
> > + | | PR Slot | |
> > + | +--------------------------+ |
> > + | Port Gasket |
> > + +------------------------------------------+
> > +
> > +Here are the major changes about DFL structures on IOFS
> > +implementation
> > design:
> > +1. The Port Gasket connects to FIU Port in DFL, but the Next_AFU
> > +pointer in FIU feature header can point to NULL so that it is no AFU
> > +connects to a FIU Port.
> > +2. The VF which include in PR region can start with AFU feature
> > +header without a FIU Port feature header.
>
> What about PF2 in static region? Which type of DFH will be used?
An IP designer may choose to add more than one PF for interfacing with IP on the FPGA.
If at least one PF implements a DFL with management features such as an FME or PR, then
the device can be managed using the IOFS software stack. For example, a design may include
FME and PR on PF0 and the actual workload interfaces on PF1. If a workload
implements virtio-net backend and interface, the IOFS software stack will only bind to and
communicate with IOFS features/interfaces found in the DFL on PF0. The second PF, PF1,
will bind with virtio-net driver presenting itself as a network interface to the OS.
So the IOFS providing the diversity for IP designer.
>
> Thanks
> Hao
>
> >
> > Performance Counters
> > ====================
> > --
> > 2.17.1
^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH v1 2/7] fpga: dfl: check feature type before parse irq info
2022-02-14 11:26 [PATCH v1 0/7] Add Intel OFS support for DFL driver Tianfei zhang
2022-02-14 11:26 ` [PATCH v1 1/7] Documentation: fpga: dfl: add description of IOFS Tianfei zhang
@ 2022-02-14 11:26 ` Tianfei zhang
2022-02-15 14:49 ` Tom Rix
2022-02-16 3:35 ` Wu, Hao
2022-02-14 11:26 ` [PATCH v1 3/7] fpga: dfl: Allow for ports with no local bar space Tianfei zhang
` (4 subsequent siblings)
6 siblings, 2 replies; 47+ messages in thread
From: Tianfei zhang @ 2022-02-14 11:26 UTC (permalink / raw)
To: hao.wu, trix, mdf, yilun.xu, linux-fpga, linux-doc, linux-kernel
Cc: corbet, Tianfei Zhang
From: Tianfei Zhang <tianfei.zhang@intel.com>
The feature ID of "Port User Interrupt" and the
"PMCI Subsystem" are identical, 0x12, but one is for FME,
other is for Port. It should check the feature type While
parsing the irq info in parse_feature_irqs().
Signed-off-by: Tianfei Zhang <tianfei.zhang@intel.com>
---
drivers/fpga/dfl.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
index 599bb21d86af..26f8cf890700 100644
--- a/drivers/fpga/dfl.c
+++ b/drivers/fpga/dfl.c
@@ -940,9 +940,14 @@ static int parse_feature_irqs(struct build_feature_devs_info *binfo,
{
void __iomem *base = binfo->ioaddr + ofst;
unsigned int i, ibase, inr = 0;
+ enum dfl_id_type type;
int virq;
u64 v;
+ type = feature_dev_id_type(binfo->feature_dev);
+ if (type >= DFL_ID_MAX)
+ return -EINVAL;
+
/*
* Ideally DFL framework should only read info from DFL header, but
* current version DFL only provides mmio resources information for
@@ -959,16 +964,22 @@ static int parse_feature_irqs(struct build_feature_devs_info *binfo,
*/
switch (fid) {
case PORT_FEATURE_ID_UINT:
+ if (type != PORT_ID)
+ break;
v = readq(base + PORT_UINT_CAP);
ibase = FIELD_GET(PORT_UINT_CAP_FST_VECT, v);
inr = FIELD_GET(PORT_UINT_CAP_INT_NUM, v);
break;
case PORT_FEATURE_ID_ERROR:
+ if (type != PORT_ID)
+ break;
v = readq(base + PORT_ERROR_CAP);
ibase = FIELD_GET(PORT_ERROR_CAP_INT_VECT, v);
inr = FIELD_GET(PORT_ERROR_CAP_SUPP_INT, v);
break;
case FME_FEATURE_ID_GLOBAL_ERR:
+ if (type != FME_ID)
+ break;
v = readq(base + FME_ERROR_CAP);
ibase = FIELD_GET(FME_ERROR_CAP_INT_VECT, v);
inr = FIELD_GET(FME_ERROR_CAP_SUPP_INT, v);
--
2.17.1
^ permalink raw reply related [flat|nested] 47+ messages in thread* Re: [PATCH v1 2/7] fpga: dfl: check feature type before parse irq info
2022-02-14 11:26 ` [PATCH v1 2/7] fpga: dfl: check feature type before parse irq info Tianfei zhang
@ 2022-02-15 14:49 ` Tom Rix
2022-02-17 2:38 ` Xu Yilun
2022-02-18 6:53 ` Zhang, Tianfei
2022-02-16 3:35 ` Wu, Hao
1 sibling, 2 replies; 47+ messages in thread
From: Tom Rix @ 2022-02-15 14:49 UTC (permalink / raw)
To: Tianfei zhang, hao.wu, mdf, yilun.xu, linux-fpga, linux-doc,
linux-kernel
Cc: corbet
On 2/14/22 3:26 AM, Tianfei zhang wrote:
> From: Tianfei Zhang <tianfei.zhang@intel.com>
>
> The feature ID of "Port User Interrupt" and the
> "PMCI Subsystem" are identical, 0x12, but one is for FME,
> other is for Port. It should check the feature type While
> parsing the irq info in parse_feature_irqs().
This seems like a bug fix and not part of iofs feature.
Split this out of the patchset.
This is a workaround a hardware problem, there should be some comments
to the effect that you can't trust _this_ or _that_ feature id and some
special handling earlier.
The ambiguity of feature id is a problem, and this sort of bug will
happen again.
What can be done to prevent this in the future ?
>
> Signed-off-by: Tianfei Zhang <tianfei.zhang@intel.com>
> ---
> drivers/fpga/dfl.c | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> index 599bb21d86af..26f8cf890700 100644
> --- a/drivers/fpga/dfl.c
> +++ b/drivers/fpga/dfl.c
> @@ -940,9 +940,14 @@ static int parse_feature_irqs(struct build_feature_devs_info *binfo,
> {
> void __iomem *base = binfo->ioaddr + ofst;
> unsigned int i, ibase, inr = 0;
> + enum dfl_id_type type;
> int virq;
> u64 v;
>
> + type = feature_dev_id_type(binfo->feature_dev);
> + if (type >= DFL_ID_MAX)
> + return -EINVAL;
> +
> /*
> * Ideally DFL framework should only read info from DFL header, but
> * current version DFL only provides mmio resources information for
> @@ -959,16 +964,22 @@ static int parse_feature_irqs(struct build_feature_devs_info *binfo,
> */
> switch (fid) {
> case PORT_FEATURE_ID_UINT:
> + if (type != PORT_ID)
> + break;
Instead of embedding a break in the switch, break the switch into fme
switch and port switch
if (type == PORT_ID)
port-switch
else if (type == FME_ID
fme-switch
Tom
> v = readq(base + PORT_UINT_CAP);
> ibase = FIELD_GET(PORT_UINT_CAP_FST_VECT, v);
> inr = FIELD_GET(PORT_UINT_CAP_INT_NUM, v);
> break;
> case PORT_FEATURE_ID_ERROR:
> + if (type != PORT_ID)
> + break;
> v = readq(base + PORT_ERROR_CAP);
> ibase = FIELD_GET(PORT_ERROR_CAP_INT_VECT, v);
> inr = FIELD_GET(PORT_ERROR_CAP_SUPP_INT, v);
> break;
> case FME_FEATURE_ID_GLOBAL_ERR:
> + if (type != FME_ID)
> + break;
> v = readq(base + FME_ERROR_CAP);
> ibase = FIELD_GET(FME_ERROR_CAP_INT_VECT, v);
> inr = FIELD_GET(FME_ERROR_CAP_SUPP_INT, v);
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH v1 2/7] fpga: dfl: check feature type before parse irq info
2022-02-15 14:49 ` Tom Rix
@ 2022-02-17 2:38 ` Xu Yilun
2022-02-21 7:54 ` Zhang, Tianfei
2022-02-18 6:53 ` Zhang, Tianfei
1 sibling, 1 reply; 47+ messages in thread
From: Xu Yilun @ 2022-02-17 2:38 UTC (permalink / raw)
To: Tom Rix
Cc: Tianfei zhang, hao.wu, mdf, linux-fpga, linux-doc, linux-kernel, corbet
On Tue, Feb 15, 2022 at 06:49:05AM -0800, Tom Rix wrote:
>
> On 2/14/22 3:26 AM, Tianfei zhang wrote:
> > From: Tianfei Zhang <tianfei.zhang@intel.com>
> >
> > The feature ID of "Port User Interrupt" and the
> > "PMCI Subsystem" are identical, 0x12, but one is for FME,
> > other is for Port. It should check the feature type While
> > parsing the irq info in parse_feature_irqs().
>
> This seems like a bug fix and not part of iofs feature.
>
> Split this out of the patchset.
>
> This is a workaround a hardware problem, there should be some comments to
> the effect that you can't trust _this_ or _that_ feature id and some special
> handling earlier.
>
> The ambiguity of feature id is a problem, and this sort of bug will happen
Actually this is not the feature id definition problem. The identity of the
feature is determined by the dfl_id_type(FME, PORT) AND feature_id. So the
driver should match the dfl_id_type & feature_id to know what feature it
is.
Thanks,
Yilun
> again.
>
> What can be done to prevent this in the future ?
>
> >
> > Signed-off-by: Tianfei Zhang <tianfei.zhang@intel.com>
> > ---
> > drivers/fpga/dfl.c | 11 +++++++++++
> > 1 file changed, 11 insertions(+)
> >
> > diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> > index 599bb21d86af..26f8cf890700 100644
> > --- a/drivers/fpga/dfl.c
> > +++ b/drivers/fpga/dfl.c
> > @@ -940,9 +940,14 @@ static int parse_feature_irqs(struct build_feature_devs_info *binfo,
> > {
> > void __iomem *base = binfo->ioaddr + ofst;
> > unsigned int i, ibase, inr = 0;
> > + enum dfl_id_type type;
> > int virq;
> > u64 v;
> > + type = feature_dev_id_type(binfo->feature_dev);
> > + if (type >= DFL_ID_MAX)
> > + return -EINVAL;
> > +
> > /*
> > * Ideally DFL framework should only read info from DFL header, but
> > * current version DFL only provides mmio resources information for
> > @@ -959,16 +964,22 @@ static int parse_feature_irqs(struct build_feature_devs_info *binfo,
> > */
> > switch (fid) {
> > case PORT_FEATURE_ID_UINT:
> > + if (type != PORT_ID)
> > + break;
>
> Instead of embedding a break in the switch, break the switch into fme switch
> and port switch
>
> if (type == PORT_ID)
>
> port-switch
>
> else if (type == FME_ID
>
> fme-switch
>
> Tom
>
> > v = readq(base + PORT_UINT_CAP);
> > ibase = FIELD_GET(PORT_UINT_CAP_FST_VECT, v);
> > inr = FIELD_GET(PORT_UINT_CAP_INT_NUM, v);
> > break;
> > case PORT_FEATURE_ID_ERROR:
> > + if (type != PORT_ID)
> > + break;
> > v = readq(base + PORT_ERROR_CAP);
> > ibase = FIELD_GET(PORT_ERROR_CAP_INT_VECT, v);
> > inr = FIELD_GET(PORT_ERROR_CAP_SUPP_INT, v);
> > break;
> > case FME_FEATURE_ID_GLOBAL_ERR:
> > + if (type != FME_ID)
> > + break;
> > v = readq(base + FME_ERROR_CAP);
> > ibase = FIELD_GET(FME_ERROR_CAP_INT_VECT, v);
> > inr = FIELD_GET(FME_ERROR_CAP_SUPP_INT, v);
^ permalink raw reply [flat|nested] 47+ messages in thread* RE: [PATCH v1 2/7] fpga: dfl: check feature type before parse irq info
2022-02-17 2:38 ` Xu Yilun
@ 2022-02-21 7:54 ` Zhang, Tianfei
0 siblings, 0 replies; 47+ messages in thread
From: Zhang, Tianfei @ 2022-02-21 7:54 UTC (permalink / raw)
To: Xu, Yilun, Tom Rix
Cc: Wu, Hao, mdf, linux-fpga, linux-doc, linux-kernel, corbet
> -----Original Message-----
> From: Xu, Yilun <yilun.xu@intel.com>
> Sent: Thursday, February 17, 2022 10:38 AM
> To: Tom Rix <trix@redhat.com>
> Cc: Zhang, Tianfei <tianfei.zhang@intel.com>; Wu, Hao <hao.wu@intel.com>;
> mdf@kernel.org; linux-fpga@vger.kernel.org; linux-doc@vger.kernel.org; linux-
> kernel@vger.kernel.org; corbet@lwn.net
> Subject: Re: [PATCH v1 2/7] fpga: dfl: check feature type before parse irq info
>
> On Tue, Feb 15, 2022 at 06:49:05AM -0800, Tom Rix wrote:
> >
> > On 2/14/22 3:26 AM, Tianfei zhang wrote:
> > > From: Tianfei Zhang <tianfei.zhang@intel.com>
> > >
> > > The feature ID of "Port User Interrupt" and the "PMCI Subsystem" are
> > > identical, 0x12, but one is for FME, other is for Port. It should
> > > check the feature type While parsing the irq info in
> > > parse_feature_irqs().
> >
> > This seems like a bug fix and not part of iofs feature.
> >
> > Split this out of the patchset.
> >
> > This is a workaround a hardware problem, there should be some comments
> > to the effect that you can't trust _this_ or _that_ feature id and
> > some special handling earlier.
> >
> > The ambiguity of feature id is a problem, and this sort of bug will
> > happen
>
> Actually this is not the feature id definition problem. The identity of the feature
> is determined by the dfl_id_type(FME, PORT) AND feature_id. So the driver
> should match the dfl_id_type & feature_id to know what feature it is.
In this function flow, create_feature_instance() -> parse_feature_irqs(), the DFL driver has not
check the dfl_id_type yet.
>
> Thanks,
> Yilun
>
> > again.
> >
> > What can be done to prevent this in the future ?
> >
> > >
> > > Signed-off-by: Tianfei Zhang <tianfei.zhang@intel.com>
> > > ---
> > > drivers/fpga/dfl.c | 11 +++++++++++
> > > 1 file changed, 11 insertions(+)
> > >
> > > diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c index
> > > 599bb21d86af..26f8cf890700 100644
> > > --- a/drivers/fpga/dfl.c
> > > +++ b/drivers/fpga/dfl.c
> > > @@ -940,9 +940,14 @@ static int parse_feature_irqs(struct
> build_feature_devs_info *binfo,
> > > {
> > > void __iomem *base = binfo->ioaddr + ofst;
> > > unsigned int i, ibase, inr = 0;
> > > + enum dfl_id_type type;
> > > int virq;
> > > u64 v;
> > > + type = feature_dev_id_type(binfo->feature_dev);
> > > + if (type >= DFL_ID_MAX)
> > > + return -EINVAL;
> > > +
> > > /*
> > > * Ideally DFL framework should only read info from DFL header, but
> > > * current version DFL only provides mmio resources information
> > > for @@ -959,16 +964,22 @@ static int parse_feature_irqs(struct
> build_feature_devs_info *binfo,
> > > */
> > > switch (fid) {
> > > case PORT_FEATURE_ID_UINT:
> > > + if (type != PORT_ID)
> > > + break;
> >
> > Instead of embedding a break in the switch, break the switch into fme
> > switch and port switch
> >
> > if (type == PORT_ID)
> >
> > port-switch
> >
> > else if (type == FME_ID
> >
> > fme-switch
> >
> > Tom
> >
> > > v = readq(base + PORT_UINT_CAP);
> > > ibase = FIELD_GET(PORT_UINT_CAP_FST_VECT, v);
> > > inr = FIELD_GET(PORT_UINT_CAP_INT_NUM, v);
> > > break;
> > > case PORT_FEATURE_ID_ERROR:
> > > + if (type != PORT_ID)
> > > + break;
> > > v = readq(base + PORT_ERROR_CAP);
> > > ibase = FIELD_GET(PORT_ERROR_CAP_INT_VECT, v);
> > > inr = FIELD_GET(PORT_ERROR_CAP_SUPP_INT, v);
> > > break;
> > > case FME_FEATURE_ID_GLOBAL_ERR:
> > > + if (type != FME_ID)
> > > + break;
> > > v = readq(base + FME_ERROR_CAP);
> > > ibase = FIELD_GET(FME_ERROR_CAP_INT_VECT, v);
> > > inr = FIELD_GET(FME_ERROR_CAP_SUPP_INT, v);
^ permalink raw reply [flat|nested] 47+ messages in thread
* RE: [PATCH v1 2/7] fpga: dfl: check feature type before parse irq info
2022-02-15 14:49 ` Tom Rix
2022-02-17 2:38 ` Xu Yilun
@ 2022-02-18 6:53 ` Zhang, Tianfei
2022-02-18 14:29 ` Tom Rix
1 sibling, 1 reply; 47+ messages in thread
From: Zhang, Tianfei @ 2022-02-18 6:53 UTC (permalink / raw)
To: Tom Rix, Wu, Hao, mdf, Xu, Yilun, linux-fpga, linux-doc, linux-kernel
Cc: corbet
> -----Original Message-----
> From: Tom Rix <trix@redhat.com>
> Sent: Tuesday, February 15, 2022 10:49 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
> Subject: Re: [PATCH v1 2/7] fpga: dfl: check feature type before parse irq info
>
>
> On 2/14/22 3:26 AM, Tianfei zhang wrote:
> > From: Tianfei Zhang <tianfei.zhang@intel.com>
> >
> > The feature ID of "Port User Interrupt" and the "PMCI Subsystem" are
> > identical, 0x12, but one is for FME, other is for Port. It should
> > check the feature type While parsing the irq info in
> > parse_feature_irqs().
>
> This seems like a bug fix and not part of iofs feature.
>
> Split this out of the patchset.
>
> This is a workaround a hardware problem, there should be some comments to
> the effect that you can't trust _this_ or _that_ feature id and some special
> handling earlier.
>
> The ambiguity of feature id is a problem, and this sort of bug will happen again.
>
> What can be done to prevent this in the future ?
This patch is not workaround, this is a bug fix for DFL driver.
The root cause is that DLF driver miss check the feature type while parsing the interrupt information,
because some Feature IDs are identical between FME and Port, like PMCI in FME and "Port User Interrupt"
in Port.
The definition of Feature ID is here:
https://github.com/OPAE/linux-dfl-feature-id/blob/master/dfl-feature-ids.rst
>
> >
> > Signed-off-by: Tianfei Zhang <tianfei.zhang@intel.com>
> > ---
> > drivers/fpga/dfl.c | 11 +++++++++++
> > 1 file changed, 11 insertions(+)
> >
> > diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c index
> > 599bb21d86af..26f8cf890700 100644
> > --- a/drivers/fpga/dfl.c
> > +++ b/drivers/fpga/dfl.c
> > @@ -940,9 +940,14 @@ static int parse_feature_irqs(struct
> build_feature_devs_info *binfo,
> > {
> > void __iomem *base = binfo->ioaddr + ofst;
> > unsigned int i, ibase, inr = 0;
> > + enum dfl_id_type type;
> > int virq;
> > u64 v;
> >
> > + type = feature_dev_id_type(binfo->feature_dev);
> > + if (type >= DFL_ID_MAX)
> > + return -EINVAL;
> > +
> > /*
> > * Ideally DFL framework should only read info from DFL header, but
> > * current version DFL only provides mmio resources information for
> > @@ -959,16 +964,22 @@ static int parse_feature_irqs(struct
> build_feature_devs_info *binfo,
> > */
> > switch (fid) {
> > case PORT_FEATURE_ID_UINT:
> > + if (type != PORT_ID)
> > + break;
>
> Instead of embedding a break in the switch, break the switch into fme switch
> and port switch
>
> if (type == PORT_ID)
>
> port-switch
>
> else if (type == FME_ID
>
> fme-switch
Your suggestion is looks good for me, I will change on next version.
>
> Tom
>
> > v = readq(base + PORT_UINT_CAP);
> > ibase = FIELD_GET(PORT_UINT_CAP_FST_VECT, v);
> > inr = FIELD_GET(PORT_UINT_CAP_INT_NUM, v);
> > break;
> > case PORT_FEATURE_ID_ERROR:
> > + if (type != PORT_ID)
> > + break;
> > v = readq(base + PORT_ERROR_CAP);
> > ibase = FIELD_GET(PORT_ERROR_CAP_INT_VECT, v);
> > inr = FIELD_GET(PORT_ERROR_CAP_SUPP_INT, v);
> > break;
> > case FME_FEATURE_ID_GLOBAL_ERR:
> > + if (type != FME_ID)
> > + break;
> > v = readq(base + FME_ERROR_CAP);
> > ibase = FIELD_GET(FME_ERROR_CAP_INT_VECT, v);
> > inr = FIELD_GET(FME_ERROR_CAP_SUPP_INT, v);
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH v1 2/7] fpga: dfl: check feature type before parse irq info
2022-02-18 6:53 ` Zhang, Tianfei
@ 2022-02-18 14:29 ` Tom Rix
2022-02-21 12:05 ` Zhang, Tianfei
0 siblings, 1 reply; 47+ messages in thread
From: Tom Rix @ 2022-02-18 14:29 UTC (permalink / raw)
To: Zhang, Tianfei, Wu, Hao, mdf, Xu, Yilun, linux-fpga, linux-doc,
linux-kernel
Cc: corbet
On 2/17/22 10:53 PM, Zhang, Tianfei wrote:
>
>> -----Original Message-----
>> From: Tom Rix <trix@redhat.com>
>> Sent: Tuesday, February 15, 2022 10:49 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
>> Subject: Re: [PATCH v1 2/7] fpga: dfl: check feature type before parse irq info
>>
>>
>> On 2/14/22 3:26 AM, Tianfei zhang wrote:
>>> From: Tianfei Zhang <tianfei.zhang@intel.com>
>>>
>>> The feature ID of "Port User Interrupt" and the "PMCI Subsystem" are
>>> identical, 0x12, but one is for FME, other is for Port. It should
>>> check the feature type While parsing the irq info in
>>> parse_feature_irqs().
>> This seems like a bug fix and not part of iofs feature.
>>
>> Split this out of the patchset.
?
>>
>> This is a workaround a hardware problem, there should be some comments to
>> the effect that you can't trust _this_ or _that_ feature id and some special
>> handling earlier.
>>
>> The ambiguity of feature id is a problem, and this sort of bug will happen again.
>>
>> What can be done to prevent this in the future ?
> This patch is not workaround, this is a bug fix for DFL driver.
> The root cause is that DLF driver miss check the feature type while parsing the interrupt information,
> because some Feature IDs are identical between FME and Port, like PMCI in FME and "Port User Interrupt"
> in Port.
> The definition of Feature ID is here:
> https://github.com/OPAE/linux-dfl-feature-id/blob/master/dfl-feature-ids.rst
Helpful but hidden. At least a link to this should be added to
Documentation/fpga/dfl.rst.
>>> Signed-off-by: Tianfei Zhang <tianfei.zhang@intel.com>
>>> ---
>>> drivers/fpga/dfl.c | 11 +++++++++++
>>> 1 file changed, 11 insertions(+)
>>>
>>> diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c index
>>> 599bb21d86af..26f8cf890700 100644
>>> --- a/drivers/fpga/dfl.c
>>> +++ b/drivers/fpga/dfl.c
>>> @@ -940,9 +940,14 @@ static int parse_feature_irqs(struct
>> build_feature_devs_info *binfo,
>>> {
>>> void __iomem *base = binfo->ioaddr + ofst;
>>> unsigned int i, ibase, inr = 0;
>>> + enum dfl_id_type type;
>>> int virq;
>>> u64 v;
>>>
>>> + type = feature_dev_id_type(binfo->feature_dev);
>>> + if (type >= DFL_ID_MAX)
>>> + return -EINVAL;
>>> +
>>> /*
>>> * Ideally DFL framework should only read info from DFL header, but
>>> * current version DFL only provides mmio resources information for
>>> @@ -959,16 +964,22 @@ static int parse_feature_irqs(struct
>> build_feature_devs_info *binfo,
>>> */
>>> switch (fid) {
>>> case PORT_FEATURE_ID_UINT:
>>> + if (type != PORT_ID)
>>> + break;
>> Instead of embedding a break in the switch, break the switch into fme switch
>> and port switch
>>
>> if (type == PORT_ID)
>>
>> port-switch
>>
>> else if (type == FME_ID
>>
>> fme-switch
> Your suggestion is looks good for me, I will change on next version.
>
>> Tom
>>
>>> v = readq(base + PORT_UINT_CAP);
>>> ibase = FIELD_GET(PORT_UINT_CAP_FST_VECT, v);
>>> inr = FIELD_GET(PORT_UINT_CAP_INT_NUM, v);
>>> break;
>>> case PORT_FEATURE_ID_ERROR:
>>> + if (type != PORT_ID)
>>> + break;
>>> v = readq(base + PORT_ERROR_CAP);
>>> ibase = FIELD_GET(PORT_ERROR_CAP_INT_VECT, v);
>>> inr = FIELD_GET(PORT_ERROR_CAP_SUPP_INT, v);
>>> break;
>>> case FME_FEATURE_ID_GLOBAL_ERR:
>>> + if (type != FME_ID)
>>> + break;
>>> v = readq(base + FME_ERROR_CAP);
>>> ibase = FIELD_GET(FME_ERROR_CAP_INT_VECT, v);
>>> inr = FIELD_GET(FME_ERROR_CAP_SUPP_INT, v);
^ permalink raw reply [flat|nested] 47+ messages in thread* RE: [PATCH v1 2/7] fpga: dfl: check feature type before parse irq info
2022-02-18 14:29 ` Tom Rix
@ 2022-02-21 12:05 ` Zhang, Tianfei
0 siblings, 0 replies; 47+ messages in thread
From: Zhang, Tianfei @ 2022-02-21 12:05 UTC (permalink / raw)
To: Tom Rix, Wu, Hao, mdf, Xu, Yilun, linux-fpga, linux-doc, linux-kernel
Cc: corbet
> -----Original Message-----
> From: Tom Rix <trix@redhat.com>
> Sent: Friday, February 18, 2022 10:30 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
> Subject: Re: [PATCH v1 2/7] fpga: dfl: check feature type before parse irq info
>
>
> On 2/17/22 10:53 PM, Zhang, Tianfei wrote:
> >
> >> -----Original Message-----
> >> From: Tom Rix <trix@redhat.com>
> >> Sent: Tuesday, February 15, 2022 10:49 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
> >> Subject: Re: [PATCH v1 2/7] fpga: dfl: check feature type before
> >> parse irq info
> >>
> >>
> >> On 2/14/22 3:26 AM, Tianfei zhang wrote:
> >>> From: Tianfei Zhang <tianfei.zhang@intel.com>
> >>>
> >>> The feature ID of "Port User Interrupt" and the "PMCI Subsystem" are
> >>> identical, 0x12, but one is for FME, other is for Port. It should
> >>> check the feature type While parsing the irq info in
> >>> parse_feature_irqs().
> >> This seems like a bug fix and not part of iofs feature.
> >>
> >> Split this out of the patchset.
>
> ?
I agree, I will send this patch as a bug fix.
>
> >>
> >> This is a workaround a hardware problem, there should be some
> >> comments to the effect that you can't trust _this_ or _that_ feature
> >> id and some special handling earlier.
> >>
> >> The ambiguity of feature id is a problem, and this sort of bug will happen
> again.
> >>
> >> What can be done to prevent this in the future ?
> > This patch is not workaround, this is a bug fix for DFL driver.
> > The root cause is that DLF driver miss check the feature type while
> > parsing the interrupt information, because some Feature IDs are identical
> between FME and Port, like PMCI in FME and "Port User Interrupt"
> > in Port.
> > The definition of Feature ID is here:
> > https://github.com/OPAE/linux-dfl-feature-id/blob/master/dfl-feature-i
> > ds.rst
> Helpful but hidden. At least a link to this should be added to
> Documentation/fpga/dfl.rst.
> >>> Signed-off-by: Tianfei Zhang <tianfei.zhang@intel.com>
> >>> ---
> >>> drivers/fpga/dfl.c | 11 +++++++++++
> >>> 1 file changed, 11 insertions(+)
> >>>
> >>> diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c index
> >>> 599bb21d86af..26f8cf890700 100644
> >>> --- a/drivers/fpga/dfl.c
> >>> +++ b/drivers/fpga/dfl.c
> >>> @@ -940,9 +940,14 @@ static int parse_feature_irqs(struct
> >> build_feature_devs_info *binfo,
> >>> {
> >>> void __iomem *base = binfo->ioaddr + ofst;
> >>> unsigned int i, ibase, inr = 0;
> >>> + enum dfl_id_type type;
> >>> int virq;
> >>> u64 v;
> >>>
> >>> + type = feature_dev_id_type(binfo->feature_dev);
> >>> + if (type >= DFL_ID_MAX)
> >>> + return -EINVAL;
> >>> +
> >>> /*
> >>> * Ideally DFL framework should only read info from DFL header, but
> >>> * current version DFL only provides mmio resources information
> >>> for @@ -959,16 +964,22 @@ static int parse_feature_irqs(struct
> >> build_feature_devs_info *binfo,
> >>> */
> >>> switch (fid) {
> >>> case PORT_FEATURE_ID_UINT:
> >>> + if (type != PORT_ID)
> >>> + break;
> >> Instead of embedding a break in the switch, break the switch into fme
> >> switch and port switch
> >>
> >> if (type == PORT_ID)
> >>
> >> port-switch
> >>
> >> else if (type == FME_ID
> >>
> >> fme-switch
> > Your suggestion is looks good for me, I will change on next version.
> >
> >> Tom
> >>
> >>> v = readq(base + PORT_UINT_CAP);
> >>> ibase = FIELD_GET(PORT_UINT_CAP_FST_VECT, v);
> >>> inr = FIELD_GET(PORT_UINT_CAP_INT_NUM, v);
> >>> break;
> >>> case PORT_FEATURE_ID_ERROR:
> >>> + if (type != PORT_ID)
> >>> + break;
> >>> v = readq(base + PORT_ERROR_CAP);
> >>> ibase = FIELD_GET(PORT_ERROR_CAP_INT_VECT, v);
> >>> inr = FIELD_GET(PORT_ERROR_CAP_SUPP_INT, v);
> >>> break;
> >>> case FME_FEATURE_ID_GLOBAL_ERR:
> >>> + if (type != FME_ID)
> >>> + break;
> >>> v = readq(base + FME_ERROR_CAP);
> >>> ibase = FIELD_GET(FME_ERROR_CAP_INT_VECT, v);
> >>> inr = FIELD_GET(FME_ERROR_CAP_SUPP_INT, v);
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v1 2/7] fpga: dfl: check feature type before parse irq info
2022-02-14 11:26 ` [PATCH v1 2/7] fpga: dfl: check feature type before parse irq info Tianfei zhang
2022-02-15 14:49 ` Tom Rix
@ 2022-02-16 3:35 ` Wu, Hao
2022-02-21 7:41 ` Zhang, Tianfei
1 sibling, 1 reply; 47+ messages in thread
From: Wu, Hao @ 2022-02-16 3:35 UTC (permalink / raw)
To: Zhang, Tianfei, trix, mdf, Xu, Yilun, linux-fpga, linux-doc,
linux-kernel
Cc: corbet
> Subject: [PATCH v1 2/7] fpga: dfl: check feature type before parse irq info
>
> From: Tianfei Zhang <tianfei.zhang@intel.com>
>
> The feature ID of "Port User Interrupt" and the
> "PMCI Subsystem" are identical, 0x12, but one is for FME,
> other is for Port. It should check the feature type While
> parsing the irq info in parse_feature_irqs().
>
> Signed-off-by: Tianfei Zhang <tianfei.zhang@intel.com>
> ---
> drivers/fpga/dfl.c | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> index 599bb21d86af..26f8cf890700 100644
> --- a/drivers/fpga/dfl.c
> +++ b/drivers/fpga/dfl.c
> @@ -940,9 +940,14 @@ static int parse_feature_irqs(struct
> build_feature_devs_info *binfo,
> {
> void __iomem *base = binfo->ioaddr + ofst;
> unsigned int i, ibase, inr = 0;
> + enum dfl_id_type type;
> int virq;
> u64 v;
>
> + type = feature_dev_id_type(binfo->feature_dev);
> + if (type >= DFL_ID_MAX)
> + return -EINVAL;
> +
You don't have to check this, it doesn't allow creating feature dev with type >= DFL_ID_MAX.
> /*
> * Ideally DFL framework should only read info from DFL header, but
> * current version DFL only provides mmio resources information for
> @@ -959,16 +964,22 @@ static int parse_feature_irqs(struct
> build_feature_devs_info *binfo,
> */
> switch (fid) {
> case PORT_FEATURE_ID_UINT:
> + if (type != PORT_ID)
> + break;
> v = readq(base + PORT_UINT_CAP);
> ibase = FIELD_GET(PORT_UINT_CAP_FST_VECT, v);
> inr = FIELD_GET(PORT_UINT_CAP_INT_NUM, v);
> break;
> case PORT_FEATURE_ID_ERROR:
> + if (type != PORT_ID)
> + break;
> v = readq(base + PORT_ERROR_CAP);
> ibase = FIELD_GET(PORT_ERROR_CAP_INT_VECT, v);
> inr = FIELD_GET(PORT_ERROR_CAP_SUPP_INT, v);
> break;
> case FME_FEATURE_ID_GLOBAL_ERR:
> + if (type != FME_ID)
> + break;
> v = readq(base + FME_ERROR_CAP);
> ibase = FIELD_GET(FME_ERROR_CAP_INT_VECT, v);
> inr = FIELD_GET(FME_ERROR_CAP_SUPP_INT, v);
> --
> 2.17.1
^ permalink raw reply [flat|nested] 47+ messages in thread* RE: [PATCH v1 2/7] fpga: dfl: check feature type before parse irq info
2022-02-16 3:35 ` Wu, Hao
@ 2022-02-21 7:41 ` Zhang, Tianfei
0 siblings, 0 replies; 47+ messages in thread
From: Zhang, Tianfei @ 2022-02-21 7:41 UTC (permalink / raw)
To: Wu, Hao, trix, mdf, Xu, Yilun, linux-fpga, linux-doc, linux-kernel; +Cc: corbet
> -----Original Message-----
> From: Wu, Hao <hao.wu@intel.com>
> Sent: Wednesday, February 16, 2022 11:35 AM
> To: Zhang, Tianfei <tianfei.zhang@intel.com>; trix@redhat.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
> Subject: Re: [PATCH v1 2/7] fpga: dfl: check feature type before parse irq info
>
> > Subject: [PATCH v1 2/7] fpga: dfl: check feature type before parse irq
> > info
> >
> > From: Tianfei Zhang <tianfei.zhang@intel.com>
> >
> > The feature ID of "Port User Interrupt" and the "PMCI Subsystem" are
> > identical, 0x12, but one is for FME, other is for Port. It should
> > check the feature type While parsing the irq info in
> > parse_feature_irqs().
> >
> > Signed-off-by: Tianfei Zhang <tianfei.zhang@intel.com>
> > ---
> > drivers/fpga/dfl.c | 11 +++++++++++
> > 1 file changed, 11 insertions(+)
> >
> > diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c index
> > 599bb21d86af..26f8cf890700 100644
> > --- a/drivers/fpga/dfl.c
> > +++ b/drivers/fpga/dfl.c
> > @@ -940,9 +940,14 @@ static int parse_feature_irqs(struct
> > build_feature_devs_info *binfo, {
> > void __iomem *base = binfo->ioaddr + ofst;
> > unsigned int i, ibase, inr = 0;
> > + enum dfl_id_type type;
> > int virq;
> > u64 v;
> >
> > + type = feature_dev_id_type(binfo->feature_dev);
> > + if (type >= DFL_ID_MAX)
> > + return -EINVAL;
> > +
>
> You don't have to check this, it doesn't allow creating feature dev with type >=
> DFL_ID_MAX.
I agree, I will fix on next version.
>
> > /*
> > * Ideally DFL framework should only read info from DFL header, but
> > * current version DFL only provides mmio resources information for
> > @@ -959,16 +964,22 @@ static int parse_feature_irqs(struct
> > build_feature_devs_info *binfo,
> > */
> > switch (fid) {
> > case PORT_FEATURE_ID_UINT:
> > + if (type != PORT_ID)
> > + break;
> > v = readq(base + PORT_UINT_CAP);
> > ibase = FIELD_GET(PORT_UINT_CAP_FST_VECT, v);
> > inr = FIELD_GET(PORT_UINT_CAP_INT_NUM, v);
> > break;
> > case PORT_FEATURE_ID_ERROR:
> > + if (type != PORT_ID)
> > + break;
> > v = readq(base + PORT_ERROR_CAP);
> > ibase = FIELD_GET(PORT_ERROR_CAP_INT_VECT, v);
> > inr = FIELD_GET(PORT_ERROR_CAP_SUPP_INT, v);
> > break;
> > case FME_FEATURE_ID_GLOBAL_ERR:
> > + if (type != FME_ID)
> > + break;
> > v = readq(base + FME_ERROR_CAP);
> > ibase = FIELD_GET(FME_ERROR_CAP_INT_VECT, v);
> > inr = FIELD_GET(FME_ERROR_CAP_SUPP_INT, v);
> > --
> > 2.17.1
^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH v1 3/7] fpga: dfl: Allow for ports with no local bar space.
2022-02-14 11:26 [PATCH v1 0/7] Add Intel OFS support for DFL driver Tianfei zhang
2022-02-14 11:26 ` [PATCH v1 1/7] Documentation: fpga: dfl: add description of IOFS Tianfei zhang
2022-02-14 11:26 ` [PATCH v1 2/7] fpga: dfl: check feature type before parse irq info Tianfei zhang
@ 2022-02-14 11:26 ` Tianfei zhang
2022-02-15 15:05 ` Tom Rix
2022-02-16 3:38 ` Wu, Hao
2022-02-14 11:26 ` [PATCH v1 4/7] fpga: dfl: fix VF creation when ports have no local BAR space Tianfei zhang
` (3 subsequent siblings)
6 siblings, 2 replies; 47+ messages in thread
From: Tianfei zhang @ 2022-02-14 11:26 UTC (permalink / raw)
To: hao.wu, trix, mdf, yilun.xu, linux-fpga, linux-doc, linux-kernel
Cc: corbet, Matthew Gerlach, Tianfei Zhang
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.
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) {
+ 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;
--
2.17.1
^ permalink raw reply related [flat|nested] 47+ messages in thread* Re: [PATCH v1 3/7] fpga: dfl: Allow for ports with no local bar space.
2022-02-14 11:26 ` [PATCH v1 3/7] fpga: dfl: Allow for ports with no local bar space Tianfei zhang
@ 2022-02-15 15:05 ` Tom Rix
2022-02-18 7:31 ` Zhang, Tianfei
2022-02-16 3:38 ` Wu, Hao
1 sibling, 1 reply; 47+ messages in thread
From: Tom Rix @ 2022-02-15 15:05 UTC (permalink / raw)
To: Tianfei zhang, hao.wu, mdf, yilun.xu, linux-fpga, linux-doc,
linux-kernel
Cc: corbet, Matthew Gerlach
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.
> 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 ?
Move up a line and skip getting an offset that will not be used.
> + 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 ?
Tom
^ permalink raw reply [flat|nested] 47+ messages in thread* RE: [PATCH v1 3/7] fpga: dfl: Allow for ports with no local bar space.
2022-02-15 15:05 ` Tom Rix
@ 2022-02-18 7:31 ` Zhang, Tianfei
2022-02-18 14:49 ` Tom Rix
0 siblings, 1 reply; 47+ messages in thread
From: Zhang, Tianfei @ 2022-02-18 7:31 UTC (permalink / raw)
To: Tom Rix, Wu, Hao, mdf, Xu, Yilun, linux-fpga, linux-doc, linux-kernel
Cc: corbet, Matthew Gerlach
> -----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
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH v1 3/7] fpga: dfl: Allow for ports with no local bar space.
2022-02-18 7:31 ` Zhang, Tianfei
@ 2022-02-18 14:49 ` Tom Rix
2022-02-21 17:22 ` matthew.gerlach
0 siblings, 1 reply; 47+ messages in thread
From: Tom Rix @ 2022-02-18 14:49 UTC (permalink / raw)
To: Zhang, Tianfei, Wu, Hao, mdf, Xu, Yilun, linux-fpga, linux-doc,
linux-kernel
Cc: corbet, Matthew Gerlach
On 2/17/22 11:31 PM, Zhang, Tianfei wrote:
>
>> -----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.
What I am looking for is how the older cards using (my term) dfl 1 will
still work with dfl 2 and vice versa.
No where do I see a version check for dfl 2 nor a pci id check so either
this just works or backward compatibility has not be considered.
Please add a backward compatibility section to the doc patch
>
>>> 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.
How do you tell between this case and broken hw ?
Tom
>> 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
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH v1 3/7] fpga: dfl: Allow for ports with no local bar space.
2022-02-18 14:49 ` Tom Rix
@ 2022-02-21 17:22 ` matthew.gerlach
2022-02-21 17:51 ` Tom Rix
0 siblings, 1 reply; 47+ messages in thread
From: matthew.gerlach @ 2022-02-21 17:22 UTC (permalink / raw)
To: Tom Rix
Cc: Zhang, Tianfei, Wu, Hao, mdf, Xu, Yilun, linux-fpga, linux-doc,
linux-kernel, corbet
On Fri, 18 Feb 2022, Tom Rix wrote:
>
> On 2/17/22 11:31 PM, Zhang, Tianfei wrote:
>>
>>> -----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.
>
> What I am looking for is how the older cards using (my term) dfl 1 will still
> work with dfl 2 and vice versa.
>
> No where do I see a version check for dfl 2 nor a pci id check so either this
> just works or backward compatibility has not be considered.
>
> Please add a backward compatibility section to the doc patch
>
>>
>>>> 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.
>
> How do you tell between this case and broken hw ?
>
> Tom
The field, FME_PORT_OFST_BAR_ID, is a three bit field, which is pretty
common for BARs on PCI. PCI_STD_NUM_BARS is defined as 6. Current HW
implementations are filing this field with the value, 7, which is close to
the suggestion of 0xff's. How about we define the following magic
number?
#define NO_LOCAL_PORT_BAR 7
We should also change the dev_info to be a dev_dbg and more precise to
something like the following:
if (bar == NO_LOCAL_PORT_BAR) {
dev_dbg(&pcidev->dev, "No local port BAR space.\n");
continue;
}
>
>>> 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
>
>
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH v1 3/7] fpga: dfl: Allow for ports with no local bar space.
2022-02-21 17:22 ` matthew.gerlach
@ 2022-02-21 17:51 ` Tom Rix
2022-02-22 9:07 ` Zhang, Tianfei
0 siblings, 1 reply; 47+ messages in thread
From: Tom Rix @ 2022-02-21 17:51 UTC (permalink / raw)
To: matthew.gerlach
Cc: Zhang, Tianfei, Wu, Hao, mdf, Xu, Yilun, linux-fpga, linux-doc,
linux-kernel, corbet
On 2/21/22 9:22 AM, matthew.gerlach@linux.intel.com wrote:
>
>
> On Fri, 18 Feb 2022, Tom Rix wrote:
>
>>
>> On 2/17/22 11:31 PM, Zhang, Tianfei wrote:
>>>
>>>> -----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.
>>
>> What I am looking for is how the older cards using (my term) dfl 1
>> will still work with dfl 2 and vice versa.
>>
>> No where do I see a version check for dfl 2 nor a pci id check so
>> either this just works or backward compatibility has not be considered.
>>
>> Please add a backward compatibility section to the doc patch
>>
>>>
>>>>> 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.
>>
>> How do you tell between this case and broken hw ?
>>
>> Tom
>
> The field, FME_PORT_OFST_BAR_ID, is a three bit field, which is pretty
> common for BARs on PCI. PCI_STD_NUM_BARS is defined as 6. Current HW
> implementations are filing this field with the value, 7, which is
> close to the suggestion of 0xff's. How about we define the following
> magic number?
> #define NO_LOCAL_PORT_BAR 7
>
> We should also change the dev_info to be a dev_dbg and more precise to
> something like the following:
>
> if (bar == NO_LOCAL_PORT_BAR) {
> dev_dbg(&pcidev->dev, "No local port BAR space.\n");
> continue;
> }
What I am looking for is way generally is to tell if this is an old
framework or a new one.
Maybe a flag and/or version added to dfl_fpga_cdev on probing ?
(The meaning of released_port_num likely needs to change there as well)
So in this case you could check if this was the new framework before
doing the bar check.
Similar checks other places where ofs stuff is being fit it.
My concern is the fitting in without checking will break the old stuff.
And why I wanted to see a probing writeup in the dfl.rst doc
Tom
>
>>
>>>> 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
>>
>>
>
^ permalink raw reply [flat|nested] 47+ messages in thread* RE: [PATCH v1 3/7] fpga: dfl: Allow for ports with no local bar space.
2022-02-21 17:51 ` Tom Rix
@ 2022-02-22 9:07 ` Zhang, Tianfei
0 siblings, 0 replies; 47+ messages in thread
From: Zhang, Tianfei @ 2022-02-22 9:07 UTC (permalink / raw)
To: Tom Rix, matthew.gerlach
Cc: Wu, Hao, mdf, Xu, Yilun, linux-fpga, linux-doc, linux-kernel, corbet
> -----Original Message-----
> From: Tom Rix <trix@redhat.com>
> Sent: Tuesday, February 22, 2022 1:52 AM
> To: matthew.gerlach@linux.intel.com
> Cc: 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; corbet@lwn.net
> Subject: Re: [PATCH v1 3/7] fpga: dfl: Allow for ports with no local bar space.
>
>
> On 2/21/22 9:22 AM, matthew.gerlach@linux.intel.com wrote:
> >
> >
> > On Fri, 18 Feb 2022, Tom Rix wrote:
> >
> >>
> >> On 2/17/22 11:31 PM, Zhang, Tianfei wrote:
> >>>
> >>>> -----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.
> >>
> >> What I am looking for is how the older cards using (my term) dfl 1
> >> will still work with dfl 2 and vice versa.
> >>
> >> No where do I see a version check for dfl 2 nor a pci id check so
> >> either this just works or backward compatibility has not be considered.
> >>
> >> Please add a backward compatibility section to the doc patch
> >>
> >>>
> >>>>> 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.
> >>
> >> How do you tell between this case and broken hw ?
> >>
> >> Tom
> >
> > The field, FME_PORT_OFST_BAR_ID, is a three bit field, which is pretty
> > common for BARs on PCI. PCI_STD_NUM_BARS is defined as 6. Current HW
> > implementations are filing this field with the value, 7, which is
> > close to the suggestion of 0xff's. How about we define the following
> > magic number?
> > #define NO_LOCAL_PORT_BAR 7
> >
> > We should also change the dev_info to be a dev_dbg and more precise to
> > something like the following:
> >
> > if (bar == NO_LOCAL_PORT_BAR) {
> > dev_dbg(&pcidev->dev, "No local port BAR space.\n");
> > continue;
> > }
>
> What I am looking for is way generally is to tell if this is an old framework or a
> new one.
>
> Maybe a flag and/or version added to dfl_fpga_cdev on probing ?
I am agree add " features" in dfl_fpga_cdev on probing, for example:
struct dfl_fpga_cdev {
...
#define DFL_FEAT_xxxx (1<<0)
u64 features;
};
>
> (The meaning of released_port_num likely needs to change there as well)
>
> So in this case you could check if this was the new framework before doing the
> bar check.
>
> Similar checks other places where ofs stuff is being fit it.
>
> My concern is the fitting in without checking will break the old stuff.
>
> And why I wanted to see a probing writeup in the dfl.rst doc
>
> Tom
>
> >
> >>
> >>>> 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
> >>
> >>
> >
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v1 3/7] fpga: dfl: Allow for ports with no local bar space.
2022-02-14 11:26 ` [PATCH v1 3/7] fpga: dfl: Allow for ports with no local bar space Tianfei zhang
2022-02-15 15:05 ` Tom Rix
@ 2022-02-16 3:38 ` Wu, Hao
2022-02-21 7:48 ` Zhang, Tianfei
1 sibling, 1 reply; 47+ messages in thread
From: Wu, Hao @ 2022-02-16 3:38 UTC (permalink / raw)
To: Zhang, Tianfei, trix, mdf, Xu, Yilun, linux-fpga, linux-doc,
linux-kernel
Cc: corbet, Matthew Gerlach
> Subject: [PATCH v1 3/7] fpga: dfl: Allow for ports with no local bar space.
>
> 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.
>
> 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);
Do we really need this info here? in FME there is one sysfs interface for 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) {
> + dev_info(&pcidev->dev, "skipping port without
> local BAR space %d\n",
> + bar);
> + continue;
Is this change for IOFS? From patch #1, we have FME and PORT on PF0, so
we should have a BAR for PORT on PF0, is my understanding correct?
Thanks
Hao
> + } 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;
>
> --
> 2.17.1
^ permalink raw reply [flat|nested] 47+ messages in thread* RE: [PATCH v1 3/7] fpga: dfl: Allow for ports with no local bar space.
2022-02-16 3:38 ` Wu, Hao
@ 2022-02-21 7:48 ` Zhang, Tianfei
0 siblings, 0 replies; 47+ messages in thread
From: Zhang, Tianfei @ 2022-02-21 7:48 UTC (permalink / raw)
To: Wu, Hao, trix, mdf, Xu, Yilun, linux-fpga, linux-doc, linux-kernel
Cc: corbet, Matthew Gerlach
> -----Original Message-----
> From: Wu, Hao <hao.wu@intel.com>
> Sent: Wednesday, February 16, 2022 11:38 AM
> To: Zhang, Tianfei <tianfei.zhang@intel.com>; trix@redhat.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.
>
> > Subject: [PATCH v1 3/7] fpga: dfl: Allow for ports with no local bar space.
> >
> > 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.
> >
> > 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);
>
> Do we really need this info here? in FME there is one sysfs interface for port
> num.
I think it is not necessary, I will remove it.
>
> > 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) {
> > + dev_info(&pcidev->dev, "skipping port without
> > local BAR space %d\n",
> > + bar);
> > + continue;
>
> Is this change for IOFS? From patch #1, we have FME and PORT on PF0, so we
> should have a BAR for PORT on PF0, is my understanding correct?
Yes, we have a Port device on each PR slot in IOFS, but for " Multiple VFs per PR slot" model,
the Port device would not connected to AFU/PR slot (the BarID of Port device should be set to invalid),
and we just can access PR slot/AFU resource via VFs.
>
> Thanks
> Hao
>
> > + } 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;
> >
> > --
> > 2.17.1
^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH v1 4/7] fpga: dfl: fix VF creation when ports have no local BAR space
2022-02-14 11:26 [PATCH v1 0/7] Add Intel OFS support for DFL driver Tianfei zhang
` (2 preceding siblings ...)
2022-02-14 11:26 ` [PATCH v1 3/7] fpga: dfl: Allow for ports with no local bar space Tianfei zhang
@ 2022-02-14 11:26 ` Tianfei zhang
2022-02-15 15:50 ` Tom Rix
2022-02-14 11:26 ` [PATCH v1 5/7] drivers: fpga: dfl: handle empty port list Tianfei zhang
` (2 subsequent siblings)
6 siblings, 1 reply; 47+ messages in thread
From: Tianfei zhang @ 2022-02-14 11:26 UTC (permalink / raw)
To: hao.wu, trix, mdf, yilun.xu, linux-fpga, linux-doc, linux-kernel
Cc: corbet, Matthew Gerlach, Tianfei Zhang
From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
When a port is not connected to the same PCIe endpoint as
the FME, the port does not need to be released before being
virtualized. Fix VF creation code to handle this new use
case.
Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
Signed-off-by: Tianfei Zhang <tianfei.zhang@intel.com>
---
drivers/fpga/dfl.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
index 26f8cf890700..cfc539a656f0 100644
--- a/drivers/fpga/dfl.c
+++ b/drivers/fpga/dfl.c
@@ -1705,15 +1705,22 @@ EXPORT_SYMBOL_GPL(dfl_fpga_cdev_config_ports_pf);
int dfl_fpga_cdev_config_ports_vf(struct dfl_fpga_cdev *cdev, int num_vfs)
{
struct dfl_feature_platform_data *pdata;
- int ret = 0;
+ int ret = 0, port_count = 0;
mutex_lock(&cdev->lock);
+
+ list_for_each_entry(pdata, &cdev->port_dev_list, node) {
+ if (pdata->dev)
+ continue;
+ port_count++;
+ }
+
/*
* can't turn multiple ports into 1 VF device, only 1 port for 1 VF
* device, so if released port number doesn't match VF device number,
* then reject the request with -EINVAL error code.
*/
- if (cdev->released_port_num != num_vfs) {
+ if (port_count && cdev->released_port_num != num_vfs) {
ret = -EINVAL;
goto done;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 47+ messages in thread* Re: [PATCH v1 4/7] fpga: dfl: fix VF creation when ports have no local BAR space
2022-02-14 11:26 ` [PATCH v1 4/7] fpga: dfl: fix VF creation when ports have no local BAR space Tianfei zhang
@ 2022-02-15 15:50 ` Tom Rix
2022-02-18 8:14 ` Zhang, Tianfei
0 siblings, 1 reply; 47+ messages in thread
From: Tom Rix @ 2022-02-15 15:50 UTC (permalink / raw)
To: Tianfei zhang, hao.wu, mdf, yilun.xu, linux-fpga, linux-doc,
linux-kernel
Cc: corbet, Matthew Gerlach
On 2/14/22 3:26 AM, Tianfei zhang wrote:
> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>
> When a port is not connected to the same PCIe endpoint as
> the FME, the port does not need to be released before being
> virtualized. Fix VF creation code to handle this new use
Similar, how does this fit in with iofs, this looks like it would not be
valid for the existing cards
> case.
>
> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> Signed-off-by: Tianfei Zhang <tianfei.zhang@intel.com>
> ---
> drivers/fpga/dfl.c | 11 +++++++++--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> index 26f8cf890700..cfc539a656f0 100644
> --- a/drivers/fpga/dfl.c
> +++ b/drivers/fpga/dfl.c
> @@ -1705,15 +1705,22 @@ EXPORT_SYMBOL_GPL(dfl_fpga_cdev_config_ports_pf);
> int dfl_fpga_cdev_config_ports_vf(struct dfl_fpga_cdev *cdev, int num_vfs)
> {
> struct dfl_feature_platform_data *pdata;
> - int ret = 0;
> + int ret = 0, port_count = 0;
>
> mutex_lock(&cdev->lock);
> +
> + list_for_each_entry(pdata, &cdev->port_dev_list, node) {
> + if (pdata->dev)
This looks wrong,
pdata->dev is dereferenced below, if there is a case were (!pdata->dev)
here there would be crash later.
> + continue;
> + port_count++;
how does this work when only some of the ports are handled in the new way ?
Tom
> + }
> +
> /*
> * can't turn multiple ports into 1 VF device, only 1 port for 1 VF
> * device, so if released port number doesn't match VF device number,
> * then reject the request with -EINVAL error code.
> */
> - if (cdev->released_port_num != num_vfs) {
> + if (port_count && cdev->released_port_num != num_vfs) {
> ret = -EINVAL;
> goto done;
> }
^ permalink raw reply [flat|nested] 47+ messages in thread* RE: [PATCH v1 4/7] fpga: dfl: fix VF creation when ports have no local BAR space
2022-02-15 15:50 ` Tom Rix
@ 2022-02-18 8:14 ` Zhang, Tianfei
2022-02-18 14:55 ` Tom Rix
0 siblings, 1 reply; 47+ messages in thread
From: Zhang, Tianfei @ 2022-02-18 8:14 UTC (permalink / raw)
To: Tom Rix, Wu, Hao, mdf, Xu, Yilun, linux-fpga, linux-doc, linux-kernel
Cc: corbet, Matthew Gerlach
> -----Original Message-----
> From: Tom Rix <trix@redhat.com>
> Sent: Tuesday, February 15, 2022 11:51 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 4/7] fpga: dfl: fix VF creation when ports have no local
> BAR space
>
>
> On 2/14/22 3:26 AM, Tianfei zhang wrote:
> > From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> >
> > When a port is not connected to the same PCIe endpoint as the FME, the
> > port does not need to be released before being virtualized. Fix VF
> > creation code to handle this new use
> Similar, how does this fit in with iofs, this looks like it would not be valid for the
> existing cards
IOFS introducing multiple methods for PR and AFU access.
1. Legacy Model.
2. Micro-Personas in AFU.
3. Multiple VFs per PR slot.
For 1 and 2 model, there are 1:1 mapping between Port device and PR slot (or entire AFU). In virtualization,
it should release the Port device firstly and then assign to VM. In this models, the DFL driver will track that
the number of port devices has released (cdev->released_port_num in dfl_fpga_cdev_config_ports_vf() function)
are equal with the numbers of SRIOV or not. But in model 3, it has multiple VFs per PR slot, and assign the VF to VM
without release the port device, so the tracking mechanism of cdev->released_port_num is not workable on new
model. This patch want to handle this new model during VF creation.
> > case.
> >
> > Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> > Signed-off-by: Tianfei Zhang <tianfei.zhang@intel.com>
> > ---
> > drivers/fpga/dfl.c | 11 +++++++++--
> > 1 file changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c index
> > 26f8cf890700..cfc539a656f0 100644
> > --- a/drivers/fpga/dfl.c
> > +++ b/drivers/fpga/dfl.c
> > @@ -1705,15 +1705,22 @@
> EXPORT_SYMBOL_GPL(dfl_fpga_cdev_config_ports_pf);
> > int dfl_fpga_cdev_config_ports_vf(struct dfl_fpga_cdev *cdev, int num_vfs)
> > {
> > struct dfl_feature_platform_data *pdata;
> > - int ret = 0;
> > + int ret = 0, port_count = 0;
> >
> > mutex_lock(&cdev->lock);
> > +
> > + list_for_each_entry(pdata, &cdev->port_dev_list, node) {
> > + if (pdata->dev)
>
> This looks wrong,
>
> pdata->dev is dereferenced below, if there is a case were (!pdata->dev)
> here there would be crash later.
>
> > + continue;
> > + port_count++;
>
> how does this work when only some of the ports are handled in the new way ?
This code want to handle the " Multiple VFs per PR slot" model as I mentioned above.
In new model, the port_count want to count that how many port devices have released.
This code looks not good readability, I try to re-write it.
>
> Tom
>
> > + }
> > +
> > /*
> > * can't turn multiple ports into 1 VF device, only 1 port for 1 VF
> > * device, so if released port number doesn't match VF device number,
> > * then reject the request with -EINVAL error code.
> > */
> > - if (cdev->released_port_num != num_vfs) {
> > + if (port_count && cdev->released_port_num != num_vfs) {
> > ret = -EINVAL;
> > goto done;
> > }
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH v1 4/7] fpga: dfl: fix VF creation when ports have no local BAR space
2022-02-18 8:14 ` Zhang, Tianfei
@ 2022-02-18 14:55 ` Tom Rix
0 siblings, 0 replies; 47+ messages in thread
From: Tom Rix @ 2022-02-18 14:55 UTC (permalink / raw)
To: Zhang, Tianfei, Wu, Hao, mdf, Xu, Yilun, linux-fpga, linux-doc,
linux-kernel
Cc: corbet, Matthew Gerlach
On 2/18/22 12:14 AM, Zhang, Tianfei wrote:
>
>> -----Original Message-----
>> From: Tom Rix <trix@redhat.com>
>> Sent: Tuesday, February 15, 2022 11:51 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 4/7] fpga: dfl: fix VF creation when ports have no local
>> BAR space
>>
>>
>> On 2/14/22 3:26 AM, Tianfei zhang wrote:
>>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>>
>>> When a port is not connected to the same PCIe endpoint as the FME, the
>>> port does not need to be released before being virtualized. Fix VF
>>> creation code to handle this new use
>> Similar, how does this fit in with iofs, this looks like it would not be valid for the
>> existing cards
> IOFS introducing multiple methods for PR and AFU access.
> 1. Legacy Model.
> 2. Micro-Personas in AFU.
> 3. Multiple VFs per PR slot.
>
> For 1 and 2 model, there are 1:1 mapping between Port device and PR slot (or entire AFU). In virtualization,
> it should release the Port device firstly and then assign to VM. In this models, the DFL driver will track that
> the number of port devices has released (cdev->released_port_num in dfl_fpga_cdev_config_ports_vf() function)
> are equal with the numbers of SRIOV or not. But in model 3, it has multiple VFs per PR slot, and assign the VF to VM
> without release the port device, so the tracking mechanism of cdev->released_port_num is not workable on new
If ->release_port_num is not workable, then it needs to be generalized.
Refactor to handle all the cases.
Tom
> model. This patch want to handle this new model during VF creation.
>
>>> case.
>>>
>>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>> Signed-off-by: Tianfei Zhang <tianfei.zhang@intel.com>
>>> ---
>>> drivers/fpga/dfl.c | 11 +++++++++--
>>> 1 file changed, 9 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c index
>>> 26f8cf890700..cfc539a656f0 100644
>>> --- a/drivers/fpga/dfl.c
>>> +++ b/drivers/fpga/dfl.c
>>> @@ -1705,15 +1705,22 @@
>> EXPORT_SYMBOL_GPL(dfl_fpga_cdev_config_ports_pf);
>>> int dfl_fpga_cdev_config_ports_vf(struct dfl_fpga_cdev *cdev, int num_vfs)
>>> {
>>> struct dfl_feature_platform_data *pdata;
>>> - int ret = 0;
>>> + int ret = 0, port_count = 0;
>>>
>>> mutex_lock(&cdev->lock);
>>> +
>>> + list_for_each_entry(pdata, &cdev->port_dev_list, node) {
>>> + if (pdata->dev)
>> This looks wrong,
>>
>> pdata->dev is dereferenced below, if there is a case were (!pdata->dev)
>> here there would be crash later.
>>
>>> + continue;
>>> + port_count++;
>> how does this work when only some of the ports are handled in the new way ?
> This code want to handle the " Multiple VFs per PR slot" model as I mentioned above.
> In new model, the port_count want to count that how many port devices have released.
> This code looks not good readability, I try to re-write it.
>
>> Tom
>>
>>> + }
>>> +
>>> /*
>>> * can't turn multiple ports into 1 VF device, only 1 port for 1 VF
>>> * device, so if released port number doesn't match VF device number,
>>> * then reject the request with -EINVAL error code.
>>> */
>>> - if (cdev->released_port_num != num_vfs) {
>>> + if (port_count && cdev->released_port_num != num_vfs) {
>>> ret = -EINVAL;
>>> goto done;
>>> }
^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH v1 5/7] drivers: fpga: dfl: handle empty port list
2022-02-14 11:26 [PATCH v1 0/7] Add Intel OFS support for DFL driver Tianfei zhang
` (3 preceding siblings ...)
2022-02-14 11:26 ` [PATCH v1 4/7] fpga: dfl: fix VF creation when ports have no local BAR space Tianfei zhang
@ 2022-02-14 11:26 ` Tianfei zhang
2022-02-15 15:55 ` Tom Rix
2022-02-14 11:26 ` [PATCH v1 6/7] fpga: dfl: Handle dfl's starting with AFU Tianfei zhang
2022-02-14 11:26 ` [PATCH v1 7/7] fpga: dfl: pci: Add generic OFS PCI PID Tianfei zhang
6 siblings, 1 reply; 47+ messages in thread
From: Tianfei zhang @ 2022-02-14 11:26 UTC (permalink / raw)
To: hao.wu, trix, mdf, yilun.xu, linux-fpga, linux-doc, linux-kernel
Cc: corbet, Matthew Gerlach, Tianfei Zhang
From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
Not all FPGA designs managed by the DFL driver have a port.
In these cases, don't write the Port Access Control register
when enabling SRIOV.
Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
Signed-off-by: Tianfei Zhang <tianfei.zhang@intel.com>
---
drivers/fpga/dfl.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
index cfc539a656f0..a5263ac258c5 100644
--- a/drivers/fpga/dfl.c
+++ b/drivers/fpga/dfl.c
@@ -1708,6 +1708,8 @@ int dfl_fpga_cdev_config_ports_vf(struct dfl_fpga_cdev *cdev, int num_vfs)
int ret = 0, port_count = 0;
mutex_lock(&cdev->lock);
+ if (list_empty(&cdev->port_dev_list))
+ goto done;
list_for_each_entry(pdata, &cdev->port_dev_list, node) {
if (pdata->dev)
--
2.17.1
^ permalink raw reply related [flat|nested] 47+ messages in thread* Re: [PATCH v1 5/7] drivers: fpga: dfl: handle empty port list
2022-02-14 11:26 ` [PATCH v1 5/7] drivers: fpga: dfl: handle empty port list Tianfei zhang
@ 2022-02-15 15:55 ` Tom Rix
2022-02-18 8:24 ` Zhang, Tianfei
0 siblings, 1 reply; 47+ messages in thread
From: Tom Rix @ 2022-02-15 15:55 UTC (permalink / raw)
To: Tianfei zhang, hao.wu, mdf, yilun.xu, linux-fpga, linux-doc,
linux-kernel
Cc: corbet, Matthew Gerlach
On 2/14/22 3:26 AM, Tianfei zhang wrote:
> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>
> Not all FPGA designs managed by the DFL driver have a port.
> In these cases, don't write the Port Access Control register
> when enabling SRIOV.
Drop the 'drivers:' in the subject line.
This patch likely needs to moved to 4/7 since the last patch also
iterated over the list.
Tom
>
> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> Signed-off-by: Tianfei Zhang <tianfei.zhang@intel.com>
> ---
> drivers/fpga/dfl.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> index cfc539a656f0..a5263ac258c5 100644
> --- a/drivers/fpga/dfl.c
> +++ b/drivers/fpga/dfl.c
> @@ -1708,6 +1708,8 @@ int dfl_fpga_cdev_config_ports_vf(struct dfl_fpga_cdev *cdev, int num_vfs)
> int ret = 0, port_count = 0;
>
> mutex_lock(&cdev->lock);
> + if (list_empty(&cdev->port_dev_list))
> + goto done;
>
> list_for_each_entry(pdata, &cdev->port_dev_list, node) {
> if (pdata->dev)
^ permalink raw reply [flat|nested] 47+ messages in thread* RE: [PATCH v1 5/7] drivers: fpga: dfl: handle empty port list
2022-02-15 15:55 ` Tom Rix
@ 2022-02-18 8:24 ` Zhang, Tianfei
0 siblings, 0 replies; 47+ messages in thread
From: Zhang, Tianfei @ 2022-02-18 8:24 UTC (permalink / raw)
To: Tom Rix, Wu, Hao, mdf, Xu, Yilun, linux-fpga, linux-doc, linux-kernel
Cc: corbet, Matthew Gerlach
> -----Original Message-----
> From: Tom Rix <trix@redhat.com>
> Sent: Tuesday, February 15, 2022 11:56 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 5/7] drivers: fpga: dfl: handle empty port list
>
>
> On 2/14/22 3:26 AM, Tianfei zhang wrote:
> > From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> >
> > Not all FPGA designs managed by the DFL driver have a port.
> > In these cases, don't write the Port Access Control register when
> > enabling SRIOV.
>
> Drop the 'drivers:' in the subject line.
Yes, I agree.
>
> This patch likely needs to moved to 4/7 since the last patch also iterated over
> the list.
Yes, I agree, I will move it on next version patch.
>
> Tom
>
> >
> > Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> > Signed-off-by: Tianfei Zhang <tianfei.zhang@intel.com>
> > ---
> > drivers/fpga/dfl.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c index
> > cfc539a656f0..a5263ac258c5 100644
> > --- a/drivers/fpga/dfl.c
> > +++ b/drivers/fpga/dfl.c
> > @@ -1708,6 +1708,8 @@ int dfl_fpga_cdev_config_ports_vf(struct
> dfl_fpga_cdev *cdev, int num_vfs)
> > int ret = 0, port_count = 0;
> >
> > mutex_lock(&cdev->lock);
> > + if (list_empty(&cdev->port_dev_list))
> > + goto done;
> >
> > list_for_each_entry(pdata, &cdev->port_dev_list, node) {
> > if (pdata->dev)
^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH v1 6/7] fpga: dfl: Handle dfl's starting with AFU
2022-02-14 11:26 [PATCH v1 0/7] Add Intel OFS support for DFL driver Tianfei zhang
` (4 preceding siblings ...)
2022-02-14 11:26 ` [PATCH v1 5/7] drivers: fpga: dfl: handle empty port list Tianfei zhang
@ 2022-02-14 11:26 ` Tianfei zhang
2022-02-15 16:09 ` Tom Rix
2022-02-14 11:26 ` [PATCH v1 7/7] fpga: dfl: pci: Add generic OFS PCI PID Tianfei zhang
6 siblings, 1 reply; 47+ messages in thread
From: Tianfei zhang @ 2022-02-14 11:26 UTC (permalink / raw)
To: hao.wu, trix, mdf, yilun.xu, linux-fpga, linux-doc, linux-kernel
Cc: corbet, Matthew Gerlach, Tianfei Zhang
From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
Allow for a Device Feature List (DFL) to start with
a Device Feature Header (DFH) of type Accelerator Function Unit (AFU)
by doing nothing. This allows for PCIe VFs to be created.
Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
Signed-off-by: Tianfei Zhang <tianfei.zhang@intel.com>
---
drivers/fpga/dfl-pci.c | 7 ++++++-
drivers/fpga/dfl.c | 23 ++++++++++++++---------
2 files changed, 20 insertions(+), 10 deletions(-)
diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c
index 8abd9b408403..83b604d6dbe6 100644
--- a/drivers/fpga/dfl-pci.c
+++ b/drivers/fpga/dfl-pci.c
@@ -277,7 +277,12 @@ static int find_dfls_by_default(struct pci_dev *pcidev,
dfl_fpga_enum_info_add_dfl(info, start, len);
} else {
- ret = -ENODEV;
+ v = readq(base + DFH);
+ if (FIELD_GET(DFH_TYPE, v) != DFH_TYPE_AFU) {
+ dev_info(&pcidev->dev, "Unknown feature type 0x%llx id 0x%llx\n",
+ FIELD_GET(DFH_TYPE, v), FIELD_GET(DFH_ID, v));
+ ret = -ENODEV;
+ }
}
/* release I/O mappings for next step enumeration */
diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
index a5263ac258c5..25bd24a4cca0 100644
--- a/drivers/fpga/dfl.c
+++ b/drivers/fpga/dfl.c
@@ -900,9 +900,11 @@ static void build_info_free(struct build_feature_devs_info *binfo)
dfl_id_free(feature_dev_id_type(binfo->feature_dev),
binfo->feature_dev->id);
- list_for_each_entry_safe(finfo, p, &binfo->sub_features, node) {
- list_del(&finfo->node);
- kfree(finfo);
+ if (!list_empty(&binfo->sub_features)) {
+ list_for_each_entry_safe(finfo, p, &binfo->sub_features, node) {
+ list_del(&finfo->node);
+ kfree(finfo);
+ }
}
}
@@ -1437,6 +1439,7 @@ dfl_fpga_feature_devs_enumerate(struct dfl_fpga_enum_info *info)
binfo->dev = info->dev;
binfo->cdev = cdev;
+ INIT_LIST_HEAD(&binfo->sub_features);
binfo->nr_irqs = info->nr_irqs;
if (info->nr_irqs)
@@ -1446,12 +1449,14 @@ dfl_fpga_feature_devs_enumerate(struct dfl_fpga_enum_info *info)
* start enumeration for all feature devices based on Device Feature
* Lists.
*/
- list_for_each_entry(dfl, &info->dfls, node) {
- ret = parse_feature_list(binfo, dfl->start, dfl->len);
- if (ret) {
- remove_feature_devs(cdev);
- build_info_free(binfo);
- goto unregister_region_exit;
+ if (!list_empty(&info->dfls)) {
+ list_for_each_entry(dfl, &info->dfls, node) {
+ ret = parse_feature_list(binfo, dfl->start, dfl->len);
+ if (ret) {
+ remove_feature_devs(cdev);
+ build_info_free(binfo);
+ goto unregister_region_exit;
+ }
}
}
--
2.17.1
^ permalink raw reply related [flat|nested] 47+ messages in thread* Re: [PATCH v1 6/7] fpga: dfl: Handle dfl's starting with AFU
2022-02-14 11:26 ` [PATCH v1 6/7] fpga: dfl: Handle dfl's starting with AFU Tianfei zhang
@ 2022-02-15 16:09 ` Tom Rix
0 siblings, 0 replies; 47+ messages in thread
From: Tom Rix @ 2022-02-15 16:09 UTC (permalink / raw)
To: Tianfei zhang, hao.wu, mdf, yilun.xu, linux-fpga, linux-doc,
linux-kernel
Cc: corbet, Matthew Gerlach
On 2/14/22 3:26 AM, Tianfei zhang wrote:
> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>
> Allow for a Device Feature List (DFL) to start with
> a Device Feature Header (DFH) of type Accelerator Function Unit (AFU)
> by doing nothing. This allows for PCIe VFs to be created.
>
> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> Signed-off-by: Tianfei Zhang <tianfei.zhang@intel.com>
> ---
> drivers/fpga/dfl-pci.c | 7 ++++++-
> drivers/fpga/dfl.c | 23 ++++++++++++++---------
> 2 files changed, 20 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c
> index 8abd9b408403..83b604d6dbe6 100644
> --- a/drivers/fpga/dfl-pci.c
> +++ b/drivers/fpga/dfl-pci.c
> @@ -277,7 +277,12 @@ static int find_dfls_by_default(struct pci_dev *pcidev,
>
> dfl_fpga_enum_info_add_dfl(info, start, len);
> } else {
> - ret = -ENODEV;
> + v = readq(base + DFH);
This isn't likely to work on older cards.
Is there there a version to key off of ?
Tom
> + if (FIELD_GET(DFH_TYPE, v) != DFH_TYPE_AFU) {
> + dev_info(&pcidev->dev, "Unknown feature type 0x%llx id 0x%llx\n",
> + FIELD_GET(DFH_TYPE, v), FIELD_GET(DFH_ID, v));
> + ret = -ENODEV;
> + }
> }
>
> /* release I/O mappings for next step enumeration */
> diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> index a5263ac258c5..25bd24a4cca0 100644
> --- a/drivers/fpga/dfl.c
> +++ b/drivers/fpga/dfl.c
> @@ -900,9 +900,11 @@ static void build_info_free(struct build_feature_devs_info *binfo)
> dfl_id_free(feature_dev_id_type(binfo->feature_dev),
> binfo->feature_dev->id);
>
> - list_for_each_entry_safe(finfo, p, &binfo->sub_features, node) {
> - list_del(&finfo->node);
> - kfree(finfo);
> + if (!list_empty(&binfo->sub_features)) {
> + list_for_each_entry_safe(finfo, p, &binfo->sub_features, node) {
> + list_del(&finfo->node);
> + kfree(finfo);
> + }
> }
> }
>
> @@ -1437,6 +1439,7 @@ dfl_fpga_feature_devs_enumerate(struct dfl_fpga_enum_info *info)
>
> binfo->dev = info->dev;
> binfo->cdev = cdev;
> + INIT_LIST_HEAD(&binfo->sub_features);
>
> binfo->nr_irqs = info->nr_irqs;
> if (info->nr_irqs)
> @@ -1446,12 +1449,14 @@ dfl_fpga_feature_devs_enumerate(struct dfl_fpga_enum_info *info)
> * start enumeration for all feature devices based on Device Feature
> * Lists.
> */
> - list_for_each_entry(dfl, &info->dfls, node) {
> - ret = parse_feature_list(binfo, dfl->start, dfl->len);
> - if (ret) {
> - remove_feature_devs(cdev);
> - build_info_free(binfo);
> - goto unregister_region_exit;
> + if (!list_empty(&info->dfls)) {
> + list_for_each_entry(dfl, &info->dfls, node) {
> + ret = parse_feature_list(binfo, dfl->start, dfl->len);
> + if (ret) {
> + remove_feature_devs(cdev);
> + build_info_free(binfo);
> + goto unregister_region_exit;
> + }
> }
> }
>
^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH v1 7/7] fpga: dfl: pci: Add generic OFS PCI PID
2022-02-14 11:26 [PATCH v1 0/7] Add Intel OFS support for DFL driver Tianfei zhang
` (5 preceding siblings ...)
2022-02-14 11:26 ` [PATCH v1 6/7] fpga: dfl: Handle dfl's starting with AFU Tianfei zhang
@ 2022-02-14 11:26 ` Tianfei zhang
2022-02-15 16:16 ` Tom Rix
6 siblings, 1 reply; 47+ messages in thread
From: Tianfei zhang @ 2022-02-14 11:26 UTC (permalink / raw)
To: hao.wu, trix, mdf, yilun.xu, linux-fpga, linux-doc, linux-kernel
Cc: corbet, Matthew Gerlach, Tianfei Zhang
From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
Add the PCI product id for an Open FPGA Stack PCI card.
Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
Signed-off-by: Tianfei Zhang <tianfei.zhang@intel.com>
---
drivers/fpga/dfl-pci.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c
index 83b604d6dbe6..cb2fbf3eb918 100644
--- a/drivers/fpga/dfl-pci.c
+++ b/drivers/fpga/dfl-pci.c
@@ -76,12 +76,14 @@ static void cci_pci_free_irq(struct pci_dev *pcidev)
#define PCIE_DEVICE_ID_INTEL_PAC_D5005 0x0B2B
#define PCIE_DEVICE_ID_SILICOM_PAC_N5010 0x1000
#define PCIE_DEVICE_ID_SILICOM_PAC_N5011 0x1001
+#define PCIE_DEVICE_ID_INTEL_OFS 0xbcce
/* VF Device */
#define PCIE_DEVICE_ID_VF_INT_5_X 0xBCBF
#define PCIE_DEVICE_ID_VF_INT_6_X 0xBCC1
#define PCIE_DEVICE_ID_VF_DSC_1_X 0x09C5
#define PCIE_DEVICE_ID_INTEL_PAC_D5005_VF 0x0B2C
+#define PCIE_DEVICE_ID_INTEL_OFS_VF 0xbccf
static struct pci_device_id cci_pcie_id_tbl[] = {
{PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCIE_DEVICE_ID_PF_INT_5_X),},
@@ -95,6 +97,8 @@ static struct pci_device_id cci_pcie_id_tbl[] = {
{PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCIE_DEVICE_ID_INTEL_PAC_D5005_VF),},
{PCI_DEVICE(PCI_VENDOR_ID_SILICOM_DENMARK, PCIE_DEVICE_ID_SILICOM_PAC_N5010),},
{PCI_DEVICE(PCI_VENDOR_ID_SILICOM_DENMARK, PCIE_DEVICE_ID_SILICOM_PAC_N5011),},
+ {PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCIE_DEVICE_ID_INTEL_OFS),},
+ {PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCIE_DEVICE_ID_INTEL_OFS_VF),},
{0,}
};
MODULE_DEVICE_TABLE(pci, cci_pcie_id_tbl);
--
2.17.1
^ permalink raw reply related [flat|nested] 47+ messages in thread* Re: [PATCH v1 7/7] fpga: dfl: pci: Add generic OFS PCI PID
2022-02-14 11:26 ` [PATCH v1 7/7] fpga: dfl: pci: Add generic OFS PCI PID Tianfei zhang
@ 2022-02-15 16:16 ` Tom Rix
2022-02-18 9:03 ` Zhang, Tianfei
0 siblings, 1 reply; 47+ messages in thread
From: Tom Rix @ 2022-02-15 16:16 UTC (permalink / raw)
To: Tianfei zhang, hao.wu, mdf, yilun.xu, linux-fpga, linux-doc,
linux-kernel
Cc: corbet, Matthew Gerlach
On 2/14/22 3:26 AM, Tianfei zhang wrote:
> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>
> Add the PCI product id for an Open FPGA Stack PCI card.
Is there a URL to the card ?
>
> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> Signed-off-by: Tianfei Zhang <tianfei.zhang@intel.com>
> ---
> drivers/fpga/dfl-pci.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c
> index 83b604d6dbe6..cb2fbf3eb918 100644
> --- a/drivers/fpga/dfl-pci.c
> +++ b/drivers/fpga/dfl-pci.c
> @@ -76,12 +76,14 @@ static void cci_pci_free_irq(struct pci_dev *pcidev)
> #define PCIE_DEVICE_ID_INTEL_PAC_D5005 0x0B2B
> #define PCIE_DEVICE_ID_SILICOM_PAC_N5010 0x1000
> #define PCIE_DEVICE_ID_SILICOM_PAC_N5011 0x1001
> +#define PCIE_DEVICE_ID_INTEL_OFS 0xbcce
INTEL_OFS is a generic name, pci id's map to specific cards
Is there a more specific name for this card ?
Tom
>
> /* VF Device */
> #define PCIE_DEVICE_ID_VF_INT_5_X 0xBCBF
> #define PCIE_DEVICE_ID_VF_INT_6_X 0xBCC1
> #define PCIE_DEVICE_ID_VF_DSC_1_X 0x09C5
> #define PCIE_DEVICE_ID_INTEL_PAC_D5005_VF 0x0B2C
> +#define PCIE_DEVICE_ID_INTEL_OFS_VF 0xbccf
>
> static struct pci_device_id cci_pcie_id_tbl[] = {
> {PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCIE_DEVICE_ID_PF_INT_5_X),},
> @@ -95,6 +97,8 @@ static struct pci_device_id cci_pcie_id_tbl[] = {
> {PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCIE_DEVICE_ID_INTEL_PAC_D5005_VF),},
> {PCI_DEVICE(PCI_VENDOR_ID_SILICOM_DENMARK, PCIE_DEVICE_ID_SILICOM_PAC_N5010),},
> {PCI_DEVICE(PCI_VENDOR_ID_SILICOM_DENMARK, PCIE_DEVICE_ID_SILICOM_PAC_N5011),},
> + {PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCIE_DEVICE_ID_INTEL_OFS),},
> + {PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCIE_DEVICE_ID_INTEL_OFS_VF),},
> {0,}
> };
> MODULE_DEVICE_TABLE(pci, cci_pcie_id_tbl);
^ permalink raw reply [flat|nested] 47+ messages in thread* RE: [PATCH v1 7/7] fpga: dfl: pci: Add generic OFS PCI PID
2022-02-15 16:16 ` Tom Rix
@ 2022-02-18 9:03 ` Zhang, Tianfei
2022-02-18 15:27 ` Tom Rix
0 siblings, 1 reply; 47+ messages in thread
From: Zhang, Tianfei @ 2022-02-18 9:03 UTC (permalink / raw)
To: Tom Rix, Wu, Hao, mdf, Xu, Yilun, linux-fpga, linux-doc, linux-kernel
Cc: corbet, Matthew Gerlach
> -----Original Message-----
> From: Tom Rix <trix@redhat.com>
> Sent: Wednesday, February 16, 2022 12:16 AM
> 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 7/7] fpga: dfl: pci: Add generic OFS PCI PID
>
>
> On 2/14/22 3:26 AM, Tianfei zhang wrote:
> > From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> >
> > Add the PCI product id for an Open FPGA Stack PCI card.
> Is there a URL to the card ?
This PCIe Device IDs have registered by Intel.
> >
> > Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> > Signed-off-by: Tianfei Zhang <tianfei.zhang@intel.com>
> > ---
> > drivers/fpga/dfl-pci.c | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c index
> > 83b604d6dbe6..cb2fbf3eb918 100644
> > --- a/drivers/fpga/dfl-pci.c
> > +++ b/drivers/fpga/dfl-pci.c
> > @@ -76,12 +76,14 @@ static void cci_pci_free_irq(struct pci_dev *pcidev)
> > #define PCIE_DEVICE_ID_INTEL_PAC_D5005 0x0B2B
> > #define PCIE_DEVICE_ID_SILICOM_PAC_N5010 0x1000
> > #define PCIE_DEVICE_ID_SILICOM_PAC_N5011 0x1001
> > +#define PCIE_DEVICE_ID_INTEL_OFS 0xbcce
>
> INTEL_OFS is a generic name, pci id's map to specific cards
>
> Is there a more specific name for this card ?
I think using INTEL_OFS is better, because INTEL_OFS is the Generic development platform can support multiple cards which using OFS specification,
like Intel PAC N6000 card.
>
> Tom
>
> >
> > /* VF Device */
> > #define PCIE_DEVICE_ID_VF_INT_5_X 0xBCBF
> > #define PCIE_DEVICE_ID_VF_INT_6_X 0xBCC1
> > #define PCIE_DEVICE_ID_VF_DSC_1_X 0x09C5
> > #define PCIE_DEVICE_ID_INTEL_PAC_D5005_VF 0x0B2C
> > +#define PCIE_DEVICE_ID_INTEL_OFS_VF 0xbccf
> >
> > static struct pci_device_id cci_pcie_id_tbl[] = {
> > {PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCIE_DEVICE_ID_PF_INT_5_X),},
> @@
> > -95,6 +97,8 @@ static struct pci_device_id cci_pcie_id_tbl[] = {
> > {PCI_DEVICE(PCI_VENDOR_ID_INTEL,
> PCIE_DEVICE_ID_INTEL_PAC_D5005_VF),},
> > {PCI_DEVICE(PCI_VENDOR_ID_SILICOM_DENMARK,
> PCIE_DEVICE_ID_SILICOM_PAC_N5010),},
> > {PCI_DEVICE(PCI_VENDOR_ID_SILICOM_DENMARK,
> > PCIE_DEVICE_ID_SILICOM_PAC_N5011),},
> > + {PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCIE_DEVICE_ID_INTEL_OFS),},
> > + {PCI_DEVICE(PCI_VENDOR_ID_INTEL,
> PCIE_DEVICE_ID_INTEL_OFS_VF),},
> > {0,}
> > };
> > MODULE_DEVICE_TABLE(pci, cci_pcie_id_tbl);
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH v1 7/7] fpga: dfl: pci: Add generic OFS PCI PID
2022-02-18 9:03 ` Zhang, Tianfei
@ 2022-02-18 15:27 ` Tom Rix
2022-02-21 17:50 ` matthew.gerlach
0 siblings, 1 reply; 47+ messages in thread
From: Tom Rix @ 2022-02-18 15:27 UTC (permalink / raw)
To: Zhang, Tianfei, Wu, Hao, mdf, Xu, Yilun, linux-fpga, linux-doc,
linux-kernel
Cc: corbet, Matthew Gerlach
On 2/18/22 1:03 AM, Zhang, Tianfei wrote:
>
>> -----Original Message-----
>> From: Tom Rix <trix@redhat.com>
>> Sent: Wednesday, February 16, 2022 12:16 AM
>> 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 7/7] fpga: dfl: pci: Add generic OFS PCI PID
>>
>>
>> On 2/14/22 3:26 AM, Tianfei zhang wrote:
>>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>>
>>> Add the PCI product id for an Open FPGA Stack PCI card.
>> Is there a URL to the card ?
> This PCIe Device IDs have registered by Intel.
A URL is useful to introduce the board, Is there one ?
>
>>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>> Signed-off-by: Tianfei Zhang <tianfei.zhang@intel.com>
>>> ---
>>> drivers/fpga/dfl-pci.c | 4 ++++
>>> 1 file changed, 4 insertions(+)
>>>
>>> diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c index
>>> 83b604d6dbe6..cb2fbf3eb918 100644
>>> --- a/drivers/fpga/dfl-pci.c
>>> +++ b/drivers/fpga/dfl-pci.c
>>> @@ -76,12 +76,14 @@ static void cci_pci_free_irq(struct pci_dev *pcidev)
>>> #define PCIE_DEVICE_ID_INTEL_PAC_D5005 0x0B2B
>>> #define PCIE_DEVICE_ID_SILICOM_PAC_N5010 0x1000
>>> #define PCIE_DEVICE_ID_SILICOM_PAC_N5011 0x1001
>>> +#define PCIE_DEVICE_ID_INTEL_OFS 0xbcce
>> INTEL_OFS is a generic name, pci id's map to specific cards
>>
>> Is there a more specific name for this card ?
> I think using INTEL_OFS is better, because INTEL_OFS is the Generic development platform can support multiple cards which using OFS specification,
> like Intel PAC N6000 card.
I would prefer something like PCIE_DEVICE_ID_INTEL_PAC_N6000 because it
follows an existing pattern. Make it easy on a developer, they will
look at their board or box, see X and try to find something similar in
the driver source.
To use OSF_ * the name needs a suffix to differentiate it from future
cards that will also use ofs.
If this really is a generic id please explain in the doc patch how every
future board with use this single id and how a driver could work around
a hw problem in a specific board with a pci id covering multiple boards.
Tom
>
>> Tom
>>
>>> /* VF Device */
>>> #define PCIE_DEVICE_ID_VF_INT_5_X 0xBCBF
>>> #define PCIE_DEVICE_ID_VF_INT_6_X 0xBCC1
>>> #define PCIE_DEVICE_ID_VF_DSC_1_X 0x09C5
>>> #define PCIE_DEVICE_ID_INTEL_PAC_D5005_VF 0x0B2C
>>> +#define PCIE_DEVICE_ID_INTEL_OFS_VF 0xbccf
>>>
>>> static struct pci_device_id cci_pcie_id_tbl[] = {
>>> {PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCIE_DEVICE_ID_PF_INT_5_X),},
>> @@
>>> -95,6 +97,8 @@ static struct pci_device_id cci_pcie_id_tbl[] = {
>>> {PCI_DEVICE(PCI_VENDOR_ID_INTEL,
>> PCIE_DEVICE_ID_INTEL_PAC_D5005_VF),},
>>> {PCI_DEVICE(PCI_VENDOR_ID_SILICOM_DENMARK,
>> PCIE_DEVICE_ID_SILICOM_PAC_N5010),},
>>> {PCI_DEVICE(PCI_VENDOR_ID_SILICOM_DENMARK,
>>> PCIE_DEVICE_ID_SILICOM_PAC_N5011),},
>>> + {PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCIE_DEVICE_ID_INTEL_OFS),},
>>> + {PCI_DEVICE(PCI_VENDOR_ID_INTEL,
>> PCIE_DEVICE_ID_INTEL_OFS_VF),},
>>> {0,}
>>> };
>>> MODULE_DEVICE_TABLE(pci, cci_pcie_id_tbl);
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH v1 7/7] fpga: dfl: pci: Add generic OFS PCI PID
2022-02-18 15:27 ` Tom Rix
@ 2022-02-21 17:50 ` matthew.gerlach
2022-02-21 18:09 ` Tom Rix
0 siblings, 1 reply; 47+ messages in thread
From: matthew.gerlach @ 2022-02-21 17:50 UTC (permalink / raw)
To: Tom Rix
Cc: Zhang, Tianfei, Wu, Hao, mdf, Xu, Yilun, linux-fpga, linux-doc,
linux-kernel, corbet
[-- Attachment #1: Type: text/plain, Size: 4257 bytes --]
On Fri, 18 Feb 2022, Tom Rix wrote:
>
> On 2/18/22 1:03 AM, Zhang, Tianfei wrote:
>>
>>> -----Original Message-----
>>> From: Tom Rix <trix@redhat.com>
>>> Sent: Wednesday, February 16, 2022 12:16 AM
>>> 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 7/7] fpga: dfl: pci: Add generic OFS PCI PID
>>>
>>>
>>> On 2/14/22 3:26 AM, Tianfei zhang wrote:
>>>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>>>
>>>> Add the PCI product id for an Open FPGA Stack PCI card.
>>> Is there a URL to the card ?
>> This PCIe Device IDs have registered by Intel.
> A URL is useful to introduce the board, Is there one ?
>>
>>>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>>> Signed-off-by: Tianfei Zhang <tianfei.zhang@intel.com>
>>>> ---
>>>> drivers/fpga/dfl-pci.c | 4 ++++
>>>> 1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c index
>>>> 83b604d6dbe6..cb2fbf3eb918 100644
>>>> --- a/drivers/fpga/dfl-pci.c
>>>> +++ b/drivers/fpga/dfl-pci.c
>>>> @@ -76,12 +76,14 @@ static void cci_pci_free_irq(struct pci_dev *pcidev)
>>>> #define PCIE_DEVICE_ID_INTEL_PAC_D5005 0x0B2B
>>>> #define PCIE_DEVICE_ID_SILICOM_PAC_N5010 0x1000
>>>> #define PCIE_DEVICE_ID_SILICOM_PAC_N5011 0x1001
>>>> +#define PCIE_DEVICE_ID_INTEL_OFS 0xbcce
>>> INTEL_OFS is a generic name, pci id's map to specific cards
>>>
>>> Is there a more specific name for this card ?
>> I think using INTEL_OFS is better, because INTEL_OFS is the Generic
>> development platform can support multiple cards which using OFS
>> specification,
>> like Intel PAC N6000 card.
>
> I would prefer something like PCIE_DEVICE_ID_INTEL_PAC_N6000 because it
> follows an existing pattern. Make it easy on a developer, they will look at
> their board or box, see X and try to find something similar in the driver
> source.
>
> To use OSF_ * the name needs a suffix to differentiate it from future cards
> that will also use ofs.
>
> If this really is a generic id please explain in the doc patch how every
> future board with use this single id and how a driver could work around a hw
> problem in a specific board with a pci id covering multiple boards.
>
> Tom
Hi Tom,
The intent is to have a generic device id that can be used with many
different boards. Currently, we have FPGA implementations for 3 different
boards using this generic id. We may need a better name for device id
than OFS. More precisely this generic device id means a PCI function that
is described by a Device Feature List (DFL). How about
PCIE_DEVICE_ID_INTEL_DFL?
With a DFL device id, the functionality of the PF/VF is determined by the
contents of the DFL. Each Device Feature Header (DFH) in the DFL has a
revision field that can be used identify "broken" hw, or new functionality
added to a feature. Additionally, since the DFL is typically used in a
FPGA, the broken hardware, can and should be fixed in most cases.
Matthew
>
>>
>>> Tom
>>>
>>>> /* VF Device */
>>>> #define PCIE_DEVICE_ID_VF_INT_5_X 0xBCBF
>>>> #define PCIE_DEVICE_ID_VF_INT_6_X 0xBCC1
>>>> #define PCIE_DEVICE_ID_VF_DSC_1_X 0x09C5
>>>> #define PCIE_DEVICE_ID_INTEL_PAC_D5005_VF 0x0B2C
>>>> +#define PCIE_DEVICE_ID_INTEL_OFS_VF 0xbccf
>>>>
>>>> static struct pci_device_id cci_pcie_id_tbl[] = {
>>>> {PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCIE_DEVICE_ID_PF_INT_5_X),},
>>> @@
>>>> -95,6 +97,8 @@ static struct pci_device_id cci_pcie_id_tbl[] = {
>>>> {PCI_DEVICE(PCI_VENDOR_ID_INTEL,
>>> PCIE_DEVICE_ID_INTEL_PAC_D5005_VF),},
>>>> {PCI_DEVICE(PCI_VENDOR_ID_SILICOM_DENMARK,
>>> PCIE_DEVICE_ID_SILICOM_PAC_N5010),},
>>>> {PCI_DEVICE(PCI_VENDOR_ID_SILICOM_DENMARK,
>>>> PCIE_DEVICE_ID_SILICOM_PAC_N5011),},
>>>> + {PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCIE_DEVICE_ID_INTEL_OFS),},
>>>> + {PCI_DEVICE(PCI_VENDOR_ID_INTEL,
>>> PCIE_DEVICE_ID_INTEL_OFS_VF),},
>>>> {0,}
>>>> };
>>>> MODULE_DEVICE_TABLE(pci, cci_pcie_id_tbl);
>
>
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH v1 7/7] fpga: dfl: pci: Add generic OFS PCI PID
2022-02-21 17:50 ` matthew.gerlach
@ 2022-02-21 18:09 ` Tom Rix
2022-02-22 3:11 ` Zhang, Tianfei
0 siblings, 1 reply; 47+ messages in thread
From: Tom Rix @ 2022-02-21 18:09 UTC (permalink / raw)
To: matthew.gerlach
Cc: Zhang, Tianfei, Wu, Hao, mdf, Xu, Yilun, linux-fpga, linux-doc,
linux-kernel, corbet
On 2/21/22 9:50 AM, matthew.gerlach@linux.intel.com wrote:
>
>
> On Fri, 18 Feb 2022, Tom Rix wrote:
>
>>
>> On 2/18/22 1:03 AM, Zhang, Tianfei wrote:
>>>
>>>> -----Original Message-----
>>>> From: Tom Rix <trix@redhat.com>
>>>> Sent: Wednesday, February 16, 2022 12:16 AM
>>>> 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 7/7] fpga: dfl: pci: Add generic OFS PCI PID
>>>>
>>>>
>>>> On 2/14/22 3:26 AM, Tianfei zhang wrote:
>>>>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>>>>
>>>>> Add the PCI product id for an Open FPGA Stack PCI card.
>>>> Is there a URL to the card ?
>>> This PCIe Device IDs have registered by Intel.
>> A URL is useful to introduce the board, Is there one ?
>>>
>>>>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>>>> Signed-off-by: Tianfei Zhang <tianfei.zhang@intel.com>
>>>>> ---
>>>>> drivers/fpga/dfl-pci.c | 4 ++++
>>>>> 1 file changed, 4 insertions(+)
>>>>>
>>>>> diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c index
>>>>> 83b604d6dbe6..cb2fbf3eb918 100644
>>>>> --- a/drivers/fpga/dfl-pci.c
>>>>> +++ b/drivers/fpga/dfl-pci.c
>>>>> @@ -76,12 +76,14 @@ static void cci_pci_free_irq(struct pci_dev
>>>>> *pcidev)
>>>>> #define PCIE_DEVICE_ID_INTEL_PAC_D5005 0x0B2B
>>>>> #define PCIE_DEVICE_ID_SILICOM_PAC_N5010 0x1000
>>>>> #define PCIE_DEVICE_ID_SILICOM_PAC_N5011 0x1001
>>>>> +#define PCIE_DEVICE_ID_INTEL_OFS 0xbcce
>>>> INTEL_OFS is a generic name, pci id's map to specific cards
>>>>
>>>> Is there a more specific name for this card ?
>>> I think using INTEL_OFS is better, because INTEL_OFS is the Generic
>>> development platform can support multiple cards which using OFS
>>> specification,
>>> like Intel PAC N6000 card.
>>
>> I would prefer something like PCIE_DEVICE_ID_INTEL_PAC_N6000 because
>> it follows an existing pattern. Make it easy on a developer, they
>> will look at their board or box, see X and try to find something
>> similar in the driver source.
>>
>> To use OSF_ * the name needs a suffix to differentiate it from future
>> cards that will also use ofs.
>>
>> If this really is a generic id please explain in the doc patch how
>> every future board with use this single id and how a driver could
>> work around a hw problem in a specific board with a pci id covering
>> multiple boards.
>>
>> Tom
>
> Hi Tom,
>
> The intent is to have a generic device id that can be used with many
> different boards. Currently, we have FPGA implementations for 3
> different boards using this generic id. We may need a better name for
> device id than OFS. More precisely this generic device id means a PCI
> function that is described by a Device Feature List (DFL). How about
> PCIE_DEVICE_ID_INTEL_DFL?
>
> With a DFL device id, the functionality of the PF/VF is determined by
> the contents of the DFL. Each Device Feature Header (DFH) in the DFL
> has a revision field that can be used identify "broken" hw, or new
> functionality added to a feature. Additionally, since the DFL is
> typically used in a FPGA, the broken hardware, can and should be fixed
> in most cases.
How is lspci supposed to work ?
A dfl set can change with fw updates and in theory different boards
could have the same set.
Tom
>
> Matthew
>>
>>>
>>>> Tom
>>>>
>>>>> /* VF Device */
>>>>> #define PCIE_DEVICE_ID_VF_INT_5_X 0xBCBF
>>>>> #define PCIE_DEVICE_ID_VF_INT_6_X 0xBCC1
>>>>> #define PCIE_DEVICE_ID_VF_DSC_1_X 0x09C5
>>>>> #define PCIE_DEVICE_ID_INTEL_PAC_D5005_VF 0x0B2C
>>>>> +#define PCIE_DEVICE_ID_INTEL_OFS_VF 0xbccf
>>>>>
>>>>> static struct pci_device_id cci_pcie_id_tbl[] = {
>>>>> {PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCIE_DEVICE_ID_PF_INT_5_X),},
>>>> @@
>>>>> -95,6 +97,8 @@ static struct pci_device_id cci_pcie_id_tbl[] = {
>>>>> {PCI_DEVICE(PCI_VENDOR_ID_INTEL,
>>>> PCIE_DEVICE_ID_INTEL_PAC_D5005_VF),},
>>>>> {PCI_DEVICE(PCI_VENDOR_ID_SILICOM_DENMARK,
>>>> PCIE_DEVICE_ID_SILICOM_PAC_N5010),},
>>>>> {PCI_DEVICE(PCI_VENDOR_ID_SILICOM_DENMARK,
>>>>> PCIE_DEVICE_ID_SILICOM_PAC_N5011),},
>>>>> + {PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCIE_DEVICE_ID_INTEL_OFS),},
>>>>> + {PCI_DEVICE(PCI_VENDOR_ID_INTEL,
>>>> PCIE_DEVICE_ID_INTEL_OFS_VF),},
>>>>> {0,}
>>>>> };
>>>>> MODULE_DEVICE_TABLE(pci, cci_pcie_id_tbl);
>>
>>
^ permalink raw reply [flat|nested] 47+ messages in thread* RE: [PATCH v1 7/7] fpga: dfl: pci: Add generic OFS PCI PID
2022-02-21 18:09 ` Tom Rix
@ 2022-02-22 3:11 ` Zhang, Tianfei
2022-02-22 16:11 ` Tom Rix
0 siblings, 1 reply; 47+ messages in thread
From: Zhang, Tianfei @ 2022-02-22 3:11 UTC (permalink / raw)
To: Tom Rix, matthew.gerlach
Cc: Wu, Hao, mdf, Xu, Yilun, linux-fpga, linux-doc, linux-kernel, corbet
> -----Original Message-----
> From: Tom Rix <trix@redhat.com>
> Sent: Tuesday, February 22, 2022 2:10 AM
> To: matthew.gerlach@linux.intel.com
> Cc: 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; corbet@lwn.net
> Subject: Re: [PATCH v1 7/7] fpga: dfl: pci: Add generic OFS PCI PID
>
>
> On 2/21/22 9:50 AM, matthew.gerlach@linux.intel.com wrote:
> >
> >
> > On Fri, 18 Feb 2022, Tom Rix wrote:
> >
> >>
> >> On 2/18/22 1:03 AM, Zhang, Tianfei wrote:
> >>>
> >>>> -----Original Message-----
> >>>> From: Tom Rix <trix@redhat.com>
> >>>> Sent: Wednesday, February 16, 2022 12:16 AM
> >>>> 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 7/7] fpga: dfl: pci: Add generic OFS PCI PID
> >>>>
> >>>>
> >>>> On 2/14/22 3:26 AM, Tianfei zhang wrote:
> >>>>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> >>>>>
> >>>>> Add the PCI product id for an Open FPGA Stack PCI card.
> >>>> Is there a URL to the card ?
> >>> This PCIe Device IDs have registered by Intel.
> >> A URL is useful to introduce the board, Is there one ?
> >>>
> >>>>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> >>>>> Signed-off-by: Tianfei Zhang <tianfei.zhang@intel.com>
> >>>>> ---
> >>>>> drivers/fpga/dfl-pci.c | 4 ++++
> >>>>> 1 file changed, 4 insertions(+)
> >>>>>
> >>>>> diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c index
> >>>>> 83b604d6dbe6..cb2fbf3eb918 100644
> >>>>> --- a/drivers/fpga/dfl-pci.c
> >>>>> +++ b/drivers/fpga/dfl-pci.c
> >>>>> @@ -76,12 +76,14 @@ static void cci_pci_free_irq(struct pci_dev
> >>>>> *pcidev)
> >>>>> #define PCIE_DEVICE_ID_INTEL_PAC_D5005 0x0B2B
> >>>>> #define PCIE_DEVICE_ID_SILICOM_PAC_N5010 0x1000
> >>>>> #define PCIE_DEVICE_ID_SILICOM_PAC_N5011 0x1001
> >>>>> +#define PCIE_DEVICE_ID_INTEL_OFS 0xbcce
> >>>> INTEL_OFS is a generic name, pci id's map to specific cards
> >>>>
> >>>> Is there a more specific name for this card ?
> >>> I think using INTEL_OFS is better, because INTEL_OFS is the Generic
> >>> development platform can support multiple cards which using OFS
> >>> specification, like Intel PAC N6000 card.
> >>
> >> I would prefer something like PCIE_DEVICE_ID_INTEL_PAC_N6000 because
> >> it follows an existing pattern. Make it easy on a developer, they
> >> will look at their board or box, see X and try to find something
> >> similar in the driver source.
> >>
> >> To use OSF_ * the name needs a suffix to differentiate it from future
> >> cards that will also use ofs.
> >>
> >> If this really is a generic id please explain in the doc patch how
> >> every future board with use this single id and how a driver could
> >> work around a hw problem in a specific board with a pci id covering
> >> multiple boards.
> >>
> >> Tom
> >
> > Hi Tom,
> >
> > The intent is to have a generic device id that can be used with many
> > different boards. Currently, we have FPGA implementations for 3
> > different boards using this generic id. We may need a better name for
> > device id than OFS. More precisely this generic device id means a PCI
> > function that is described by a Device Feature List (DFL). How about
> > PCIE_DEVICE_ID_INTEL_DFL?
> >
> > With a DFL device id, the functionality of the PF/VF is determined by
> > the contents of the DFL. Each Device Feature Header (DFH) in the DFL
> > has a revision field that can be used identify "broken" hw, or new
> > functionality added to a feature. Additionally, since the DFL is
> > typically used in a FPGA, the broken hardware, can and should be fixed
> > in most cases.
>
> How is lspci supposed to work ?
There is an example for one card using IOFS and DFL.
# lspci | grep acc
b1:00.0 Processing accelerators: Intel Corporation Device bcce (rev 01)
b1:00.1 Processing accelerators: Intel Corporation Device bcce
b1:00.2 Processing accelerators: Intel Corporation Device bcce
b1:00.3 Processing accelerators: Red Hat, Inc. Virtio network device
b1:00.4 Processing accelerators: Intel Corporation Device bcce
Note: There 5 PFs in this card, it exports the management functions via PF0(b1:00.0),
Other PFs like b1:00.1, b1:00.2, b1:00.4, are using for testing, which depends on RTL designer
or project requirement. The PF3 instance a VirtIO net device for example, will bind with virtio-net driver
presenting itself as a network interface to the OS.
>
> A dfl set can change with fw updates and in theory different boards could have
> the same set.
>
> Tom
>
> >
> > Matthew
> >>
> >>>
> >>>> Tom
> >>>>
> >>>>> /* VF Device */
> >>>>> #define PCIE_DEVICE_ID_VF_INT_5_X 0xBCBF
> >>>>> #define PCIE_DEVICE_ID_VF_INT_6_X 0xBCC1
> >>>>> #define PCIE_DEVICE_ID_VF_DSC_1_X 0x09C5
> >>>>> #define PCIE_DEVICE_ID_INTEL_PAC_D5005_VF 0x0B2C
> >>>>> +#define PCIE_DEVICE_ID_INTEL_OFS_VF 0xbccf
> >>>>>
> >>>>> static struct pci_device_id cci_pcie_id_tbl[] = {
> >>>>> {PCI_DEVICE(PCI_VENDOR_ID_INTEL,
> >>>>> PCIE_DEVICE_ID_PF_INT_5_X),},
> >>>> @@
> >>>>> -95,6 +97,8 @@ static struct pci_device_id cci_pcie_id_tbl[] = {
> >>>>> {PCI_DEVICE(PCI_VENDOR_ID_INTEL,
> >>>> PCIE_DEVICE_ID_INTEL_PAC_D5005_VF),},
> >>>>> {PCI_DEVICE(PCI_VENDOR_ID_SILICOM_DENMARK,
> >>>> PCIE_DEVICE_ID_SILICOM_PAC_N5010),},
> >>>>> {PCI_DEVICE(PCI_VENDOR_ID_SILICOM_DENMARK,
> >>>>> PCIE_DEVICE_ID_SILICOM_PAC_N5011),},
> >>>>> + {PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCIE_DEVICE_ID_INTEL_OFS),},
> >>>>> + {PCI_DEVICE(PCI_VENDOR_ID_INTEL,
> >>>> PCIE_DEVICE_ID_INTEL_OFS_VF),},
> >>>>> {0,}
> >>>>> };
> >>>>> MODULE_DEVICE_TABLE(pci, cci_pcie_id_tbl);
> >>
> >>
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH v1 7/7] fpga: dfl: pci: Add generic OFS PCI PID
2022-02-22 3:11 ` Zhang, Tianfei
@ 2022-02-22 16:11 ` Tom Rix
2022-02-23 1:48 ` Zhang, Tianfei
2022-02-24 17:54 ` matthew.gerlach
0 siblings, 2 replies; 47+ messages in thread
From: Tom Rix @ 2022-02-22 16:11 UTC (permalink / raw)
To: Zhang, Tianfei, matthew.gerlach
Cc: Wu, Hao, mdf, Xu, Yilun, linux-fpga, linux-doc, linux-kernel, corbet
On 2/21/22 7:11 PM, Zhang, Tianfei wrote:
>
>> -----Original Message-----
>> From: Tom Rix <trix@redhat.com>
>> Sent: Tuesday, February 22, 2022 2:10 AM
>> To: matthew.gerlach@linux.intel.com
>> Cc: 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; corbet@lwn.net
>> Subject: Re: [PATCH v1 7/7] fpga: dfl: pci: Add generic OFS PCI PID
>>
>>
>> On 2/21/22 9:50 AM, matthew.gerlach@linux.intel.com wrote:
>>>
>>> On Fri, 18 Feb 2022, Tom Rix wrote:
>>>
>>>> On 2/18/22 1:03 AM, Zhang, Tianfei wrote:
>>>>>> -----Original Message-----
>>>>>> From: Tom Rix <trix@redhat.com>
>>>>>> Sent: Wednesday, February 16, 2022 12:16 AM
>>>>>> 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 7/7] fpga: dfl: pci: Add generic OFS PCI PID
>>>>>>
>>>>>>
>>>>>> On 2/14/22 3:26 AM, Tianfei zhang wrote:
>>>>>>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>>>>>>
>>>>>>> Add the PCI product id for an Open FPGA Stack PCI card.
>>>>>> Is there a URL to the card ?
>>>>> This PCIe Device IDs have registered by Intel.
>>>> A URL is useful to introduce the board, Is there one ?
>>>>>>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>>>>>> Signed-off-by: Tianfei Zhang <tianfei.zhang@intel.com>
>>>>>>> ---
>>>>>>> drivers/fpga/dfl-pci.c | 4 ++++
>>>>>>> 1 file changed, 4 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c index
>>>>>>> 83b604d6dbe6..cb2fbf3eb918 100644
>>>>>>> --- a/drivers/fpga/dfl-pci.c
>>>>>>> +++ b/drivers/fpga/dfl-pci.c
>>>>>>> @@ -76,12 +76,14 @@ static void cci_pci_free_irq(struct pci_dev
>>>>>>> *pcidev)
>>>>>>> #define PCIE_DEVICE_ID_INTEL_PAC_D5005 0x0B2B
>>>>>>> #define PCIE_DEVICE_ID_SILICOM_PAC_N5010 0x1000
>>>>>>> #define PCIE_DEVICE_ID_SILICOM_PAC_N5011 0x1001
>>>>>>> +#define PCIE_DEVICE_ID_INTEL_OFS 0xbcce
>>>>>> INTEL_OFS is a generic name, pci id's map to specific cards
>>>>>>
>>>>>> Is there a more specific name for this card ?
>>>>> I think using INTEL_OFS is better, because INTEL_OFS is the Generic
>>>>> development platform can support multiple cards which using OFS
>>>>> specification, like Intel PAC N6000 card.
>>>> I would prefer something like PCIE_DEVICE_ID_INTEL_PAC_N6000 because
>>>> it follows an existing pattern. Make it easy on a developer, they
>>>> will look at their board or box, see X and try to find something
>>>> similar in the driver source.
>>>>
>>>> To use OSF_ * the name needs a suffix to differentiate it from future
>>>> cards that will also use ofs.
>>>>
>>>> If this really is a generic id please explain in the doc patch how
>>>> every future board with use this single id and how a driver could
>>>> work around a hw problem in a specific board with a pci id covering
>>>> multiple boards.
>>>>
>>>> Tom
>>> Hi Tom,
>>>
>>> The intent is to have a generic device id that can be used with many
>>> different boards. Currently, we have FPGA implementations for 3
>>> different boards using this generic id. We may need a better name for
>>> device id than OFS. More precisely this generic device id means a PCI
>>> function that is described by a Device Feature List (DFL). How about
>>> PCIE_DEVICE_ID_INTEL_DFL?
>>>
>>> With a DFL device id, the functionality of the PF/VF is determined by
>>> the contents of the DFL. Each Device Feature Header (DFH) in the DFL
>>> has a revision field that can be used identify "broken" hw, or new
>>> functionality added to a feature. Additionally, since the DFL is
>>> typically used in a FPGA, the broken hardware, can and should be fixed
>>> in most cases.
>> How is lspci supposed to work ?
> There is an example for one card using IOFS and DFL.
>
> # lspci | grep acc
> b1:00.0 Processing accelerators: Intel Corporation Device bcce (rev 01)
> b1:00.1 Processing accelerators: Intel Corporation Device bcce
> b1:00.2 Processing accelerators: Intel Corporation Device bcce
> b1:00.3 Processing accelerators: Red Hat, Inc. Virtio network device
> b1:00.4 Processing accelerators: Intel Corporation Device bcce
>
> Note: There 5 PFs in this card, it exports the management functions via PF0(b1:00.0),
> Other PFs like b1:00.1, b1:00.2, b1:00.4, are using for testing, which depends on RTL designer
> or project requirement. The PF3 instance a VirtIO net device for example, will bind with virtio-net driver
> presenting itself as a network interface to the OS.
What I mean there is heterogeneous set of cards in one machine, how do
you tell which card is which ?
Or in a datacenter where the machines are all remote and admin has to
flash just the n6000's ?
How could she find just the n6000's with lspci ?
How would the driver know ?
Tom
>
>> A dfl set can change with fw updates and in theory different boards could have
>> the same set.
>>
>> Tom
>>
>>> Matthew
>>>>>> Tom
>>>>>>
>>>>>>> /* VF Device */
>>>>>>> #define PCIE_DEVICE_ID_VF_INT_5_X 0xBCBF
>>>>>>> #define PCIE_DEVICE_ID_VF_INT_6_X 0xBCC1
>>>>>>> #define PCIE_DEVICE_ID_VF_DSC_1_X 0x09C5
>>>>>>> #define PCIE_DEVICE_ID_INTEL_PAC_D5005_VF 0x0B2C
>>>>>>> +#define PCIE_DEVICE_ID_INTEL_OFS_VF 0xbccf
>>>>>>>
>>>>>>> static struct pci_device_id cci_pcie_id_tbl[] = {
>>>>>>> {PCI_DEVICE(PCI_VENDOR_ID_INTEL,
>>>>>>> PCIE_DEVICE_ID_PF_INT_5_X),},
>>>>>> @@
>>>>>>> -95,6 +97,8 @@ static struct pci_device_id cci_pcie_id_tbl[] = {
>>>>>>> {PCI_DEVICE(PCI_VENDOR_ID_INTEL,
>>>>>> PCIE_DEVICE_ID_INTEL_PAC_D5005_VF),},
>>>>>>> {PCI_DEVICE(PCI_VENDOR_ID_SILICOM_DENMARK,
>>>>>> PCIE_DEVICE_ID_SILICOM_PAC_N5010),},
>>>>>>> {PCI_DEVICE(PCI_VENDOR_ID_SILICOM_DENMARK,
>>>>>>> PCIE_DEVICE_ID_SILICOM_PAC_N5011),},
>>>>>>> + {PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCIE_DEVICE_ID_INTEL_OFS),},
>>>>>>> + {PCI_DEVICE(PCI_VENDOR_ID_INTEL,
>>>>>> PCIE_DEVICE_ID_INTEL_OFS_VF),},
>>>>>>> {0,}
>>>>>>> };
>>>>>>> MODULE_DEVICE_TABLE(pci, cci_pcie_id_tbl);
>>>>
^ permalink raw reply [flat|nested] 47+ messages in thread* RE: [PATCH v1 7/7] fpga: dfl: pci: Add generic OFS PCI PID
2022-02-22 16:11 ` Tom Rix
@ 2022-02-23 1:48 ` Zhang, Tianfei
2022-02-24 17:54 ` matthew.gerlach
1 sibling, 0 replies; 47+ messages in thread
From: Zhang, Tianfei @ 2022-02-23 1:48 UTC (permalink / raw)
To: Tom Rix, matthew.gerlach
Cc: Wu, Hao, mdf, Xu, Yilun, linux-fpga, linux-doc, linux-kernel, corbet
> -----Original Message-----
> From: Tom Rix <trix@redhat.com>
> Sent: Wednesday, February 23, 2022 12:11 AM
> To: Zhang, Tianfei <tianfei.zhang@intel.com>;
> matthew.gerlach@linux.intel.com
> Cc: 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; corbet@lwn.net
> Subject: Re: [PATCH v1 7/7] fpga: dfl: pci: Add generic OFS PCI PID
>
>
> On 2/21/22 7:11 PM, Zhang, Tianfei wrote:
> >
> >> -----Original Message-----
> >> From: Tom Rix <trix@redhat.com>
> >> Sent: Tuesday, February 22, 2022 2:10 AM
> >> To: matthew.gerlach@linux.intel.com
> >> Cc: 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; corbet@lwn.net
> >> Subject: Re: [PATCH v1 7/7] fpga: dfl: pci: Add generic OFS PCI PID
> >>
> >>
> >> On 2/21/22 9:50 AM, matthew.gerlach@linux.intel.com wrote:
> >>>
> >>> On Fri, 18 Feb 2022, Tom Rix wrote:
> >>>
> >>>> On 2/18/22 1:03 AM, Zhang, Tianfei wrote:
> >>>>>> -----Original Message-----
> >>>>>> From: Tom Rix <trix@redhat.com>
> >>>>>> Sent: Wednesday, February 16, 2022 12:16 AM
> >>>>>> 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 7/7] fpga: dfl: pci: Add generic OFS PCI
> >>>>>> PID
> >>>>>>
> >>>>>>
> >>>>>> On 2/14/22 3:26 AM, Tianfei zhang wrote:
> >>>>>>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> >>>>>>>
> >>>>>>> Add the PCI product id for an Open FPGA Stack PCI card.
> >>>>>> Is there a URL to the card ?
> >>>>> This PCIe Device IDs have registered by Intel.
> >>>> A URL is useful to introduce the board, Is there one ?
> >>>>>>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> >>>>>>> Signed-off-by: Tianfei Zhang <tianfei.zhang@intel.com>
> >>>>>>> ---
> >>>>>>> drivers/fpga/dfl-pci.c | 4 ++++
> >>>>>>> 1 file changed, 4 insertions(+)
> >>>>>>>
> >>>>>>> diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c
> >>>>>>> index
> >>>>>>> 83b604d6dbe6..cb2fbf3eb918 100644
> >>>>>>> --- a/drivers/fpga/dfl-pci.c
> >>>>>>> +++ b/drivers/fpga/dfl-pci.c
> >>>>>>> @@ -76,12 +76,14 @@ static void cci_pci_free_irq(struct pci_dev
> >>>>>>> *pcidev)
> >>>>>>> #define PCIE_DEVICE_ID_INTEL_PAC_D5005 0x0B2B
> >>>>>>> #define PCIE_DEVICE_ID_SILICOM_PAC_N5010 0x1000
> >>>>>>> #define PCIE_DEVICE_ID_SILICOM_PAC_N5011 0x1001
> >>>>>>> +#define PCIE_DEVICE_ID_INTEL_OFS 0xbcce
> >>>>>> INTEL_OFS is a generic name, pci id's map to specific cards
> >>>>>>
> >>>>>> Is there a more specific name for this card ?
> >>>>> I think using INTEL_OFS is better, because INTEL_OFS is the
> >>>>> Generic development platform can support multiple cards which
> >>>>> using OFS specification, like Intel PAC N6000 card.
> >>>> I would prefer something like PCIE_DEVICE_ID_INTEL_PAC_N6000
> >>>> because it follows an existing pattern. Make it easy on a
> >>>> developer, they will look at their board or box, see X and try to
> >>>> find something similar in the driver source.
> >>>>
> >>>> To use OSF_ * the name needs a suffix to differentiate it from
> >>>> future cards that will also use ofs.
> >>>>
> >>>> If this really is a generic id please explain in the doc patch how
> >>>> every future board with use this single id and how a driver could
> >>>> work around a hw problem in a specific board with a pci id covering
> >>>> multiple boards.
> >>>>
> >>>> Tom
> >>> Hi Tom,
> >>>
> >>> The intent is to have a generic device id that can be used with many
> >>> different boards. Currently, we have FPGA implementations for 3
> >>> different boards using this generic id. We may need a better name
> >>> for device id than OFS. More precisely this generic device id means
> >>> a PCI function that is described by a Device Feature List (DFL).
> >>> How about PCIE_DEVICE_ID_INTEL_DFL?
> >>>
> >>> With a DFL device id, the functionality of the PF/VF is determined
> >>> by the contents of the DFL. Each Device Feature Header (DFH) in the
> >>> DFL has a revision field that can be used identify "broken" hw, or
> >>> new functionality added to a feature. Additionally, since the DFL
> >>> is typically used in a FPGA, the broken hardware, can and should be
> >>> fixed in most cases.
> >> How is lspci supposed to work ?
> > There is an example for one card using IOFS and DFL.
> >
> > # lspci | grep acc
> > b1:00.0 Processing accelerators: Intel Corporation Device bcce (rev
> > 01)
> > b1:00.1 Processing accelerators: Intel Corporation Device bcce
> > b1:00.2 Processing accelerators: Intel Corporation Device bcce
> > b1:00.3 Processing accelerators: Red Hat, Inc. Virtio network device
> > b1:00.4 Processing accelerators: Intel Corporation Device bcce
> >
> > Note: There 5 PFs in this card, it exports the management functions
> > via PF0(b1:00.0), Other PFs like b1:00.1, b1:00.2, b1:00.4, are using
> > for testing, which depends on RTL designer or project requirement. The
> > PF3 instance a VirtIO net device for example, will bind with virtio-net driver
> presenting itself as a network interface to the OS.
>
> What I mean there is heterogeneous set of cards in one machine, how do you
> tell which card is which ?
>
> Or in a datacenter where the machines are all remote and admin has to flash just
> the n6000's ?
>
> How could she find just the n6000's with lspci ?
This is good question, we have several methods to distinguish in heterogeneous set of cards：
1. BDF for card, each card or each PF/VF has different BDF on PCIe bus.
When we want to flash the card, it need to specify the BDF of card. For example, our OPAE userspace tool
" fpgasupdate" need to provide the BDF of the card in the argument.
2. if we have several different cards with the same PCIE PID which implemented the IOFS specification, some of
ID will be different, for example, the Bitstream Id, Pr Interface Id, the BMC FW version, BMC build version and so on,
Those information exposed by sysfs node. For example, here is the sysfs node for Pr Interface Id:
/sys/class/fpga_region/regionX/dfl-fme.X/dfl-fme-region.X/fpga_region/regionX/ compat_id
The end-user can easy using OPAE tools to show this information, like fpgainfo.
[root@ ]# fpgainfo fme
Intel N6000 Acceleration Development Platform
Board Management Controller, MAX10 NIOS FW version: 3.4.0
Board Management Controller, MAX10 Build version: 3.4.0
//****** FME ******//
Object Id : 0xEF00000
PCIe s:b:d.f : 0000:B1:00.0
Device Id : 0xBCCE
Socket Id : 0x00
Ports Num : 01
Bitstream Id : 0x50104022AC010D2
Bitstream Version : 5.0.1
Pr Interface Id : bb03eb0e-4a61-547f-a0ce-28ffe8ab25f3
Boot Page : user1
Factory Image Info : a0ce28ffe8ab25f3bb03eb0e4a61547f
User1 Image Info : a0ce28ffe8ab25f3bb03eb0e4a61547f
User2 Image Info : a0ce28ffe8ab25f3bb03eb0e4a61547f
>
> How would the driver know ?
>
> Tom
>
> >
> >> A dfl set can change with fw updates and in theory different boards
> >> could have the same set.
> >>
> >> Tom
> >>
> >>> Matthew
> >>>>>> Tom
> >>>>>>
> >>>>>>> /* VF Device */
> >>>>>>> #define PCIE_DEVICE_ID_VF_INT_5_X 0xBCBF
> >>>>>>> #define PCIE_DEVICE_ID_VF_INT_6_X 0xBCC1
> >>>>>>> #define PCIE_DEVICE_ID_VF_DSC_1_X 0x09C5
> >>>>>>> #define PCIE_DEVICE_ID_INTEL_PAC_D5005_VF 0x0B2C
> >>>>>>> +#define PCIE_DEVICE_ID_INTEL_OFS_VF 0xbccf
> >>>>>>>
> >>>>>>> static struct pci_device_id cci_pcie_id_tbl[] = {
> >>>>>>> {PCI_DEVICE(PCI_VENDOR_ID_INTEL,
> >>>>>>> PCIE_DEVICE_ID_PF_INT_5_X),},
> >>>>>> @@
> >>>>>>> -95,6 +97,8 @@ static struct pci_device_id cci_pcie_id_tbl[] = {
> >>>>>>> {PCI_DEVICE(PCI_VENDOR_ID_INTEL,
> >>>>>> PCIE_DEVICE_ID_INTEL_PAC_D5005_VF),},
> >>>>>>> {PCI_DEVICE(PCI_VENDOR_ID_SILICOM_DENMARK,
> >>>>>> PCIE_DEVICE_ID_SILICOM_PAC_N5010),},
> >>>>>>> {PCI_DEVICE(PCI_VENDOR_ID_SILICOM_DENMARK,
> >>>>>>> PCIE_DEVICE_ID_SILICOM_PAC_N5011),},
> >>>>>>> + {PCI_DEVICE(PCI_VENDOR_ID_INTEL,
> >>>>>>> +PCIE_DEVICE_ID_INTEL_OFS),},
> >>>>>>> + {PCI_DEVICE(PCI_VENDOR_ID_INTEL,
> >>>>>> PCIE_DEVICE_ID_INTEL_OFS_VF),},
> >>>>>>> {0,}
> >>>>>>> };
> >>>>>>> MODULE_DEVICE_TABLE(pci, cci_pcie_id_tbl);
> >>>>
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH v1 7/7] fpga: dfl: pci: Add generic OFS PCI PID
2022-02-22 16:11 ` Tom Rix
2022-02-23 1:48 ` Zhang, Tianfei
@ 2022-02-24 17:54 ` matthew.gerlach
2022-02-28 10:57 ` Wu, Hao
1 sibling, 1 reply; 47+ messages in thread
From: matthew.gerlach @ 2022-02-24 17:54 UTC (permalink / raw)
To: Tom Rix
Cc: Zhang, Tianfei, Wu, Hao, mdf, Xu, Yilun, linux-fpga, linux-doc,
linux-kernel, corbet, aaron.j.grier
[-- Attachment #1: Type: text/plain, Size: 8105 bytes --]
On Tue, 22 Feb 2022, Tom Rix wrote:
>
> On 2/21/22 7:11 PM, Zhang, Tianfei wrote:
>>
>>> -----Original Message-----
>>> From: Tom Rix <trix@redhat.com>
>>> Sent: Tuesday, February 22, 2022 2:10 AM
>>> To: matthew.gerlach@linux.intel.com
>>> Cc: 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; corbet@lwn.net
>>> Subject: Re: [PATCH v1 7/7] fpga: dfl: pci: Add generic OFS PCI PID
>>>
>>>
>>> On 2/21/22 9:50 AM, matthew.gerlach@linux.intel.com wrote:
>>>>
>>>> On Fri, 18 Feb 2022, Tom Rix wrote:
>>>>
>>>>> On 2/18/22 1:03 AM, Zhang, Tianfei wrote:
>>>>>>> -----Original Message-----
>>>>>>> From: Tom Rix <trix@redhat.com>
>>>>>>> Sent: Wednesday, February 16, 2022 12:16 AM
>>>>>>> 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 7/7] fpga: dfl: pci: Add generic OFS PCI PID
>>>>>>>
>>>>>>>
>>>>>>> On 2/14/22 3:26 AM, Tianfei zhang wrote:
>>>>>>>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>>>>>>>
>>>>>>>> Add the PCI product id for an Open FPGA Stack PCI card.
>>>>>>> Is there a URL to the card ?
>>>>>> This PCIe Device IDs have registered by Intel.
>>>>> A URL is useful to introduce the board, Is there one ?
>>>>>>>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>>>>>>> Signed-off-by: Tianfei Zhang <tianfei.zhang@intel.com>
>>>>>>>> ---
>>>>>>>> drivers/fpga/dfl-pci.c | 4 ++++
>>>>>>>> 1 file changed, 4 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c index
>>>>>>>> 83b604d6dbe6..cb2fbf3eb918 100644
>>>>>>>> --- a/drivers/fpga/dfl-pci.c
>>>>>>>> +++ b/drivers/fpga/dfl-pci.c
>>>>>>>> @@ -76,12 +76,14 @@ static void cci_pci_free_irq(struct pci_dev
>>>>>>>> *pcidev)
>>>>>>>> #define PCIE_DEVICE_ID_INTEL_PAC_D5005 0x0B2B
>>>>>>>> #define PCIE_DEVICE_ID_SILICOM_PAC_N5010 0x1000
>>>>>>>> #define PCIE_DEVICE_ID_SILICOM_PAC_N5011 0x1001
>>>>>>>> +#define PCIE_DEVICE_ID_INTEL_OFS 0xbcce
>>>>>>> INTEL_OFS is a generic name, pci id's map to specific cards
>>>>>>>
>>>>>>> Is there a more specific name for this card ?
>>>>>> I think using INTEL_OFS is better, because INTEL_OFS is the Generic
>>>>>> development platform can support multiple cards which using OFS
>>>>>> specification, like Intel PAC N6000 card.
>>>>> I would prefer something like PCIE_DEVICE_ID_INTEL_PAC_N6000 because
>>>>> it follows an existing pattern. Make it easy on a developer, they
>>>>> will look at their board or box, see X and try to find something
>>>>> similar in the driver source.
>>>>>
>>>>> To use OSF_ * the name needs a suffix to differentiate it from future
>>>>> cards that will also use ofs.
>>>>>
>>>>> If this really is a generic id please explain in the doc patch how
>>>>> every future board with use this single id and how a driver could
>>>>> work around a hw problem in a specific board with a pci id covering
>>>>> multiple boards.
>>>>>
>>>>> Tom
>>>> Hi Tom,
>>>>
>>>> The intent is to have a generic device id that can be used with many
>>>> different boards. Currently, we have FPGA implementations for 3
>>>> different boards using this generic id. We may need a better name for
>>>> device id than OFS. More precisely this generic device id means a PCI
>>>> function that is described by a Device Feature List (DFL). How about
>>>> PCIE_DEVICE_ID_INTEL_DFL?
>>>>
>>>> With a DFL device id, the functionality of the PF/VF is determined by
>>>> the contents of the DFL. Each Device Feature Header (DFH) in the DFL
>>>> has a revision field that can be used identify "broken" hw, or new
>>>> functionality added to a feature. Additionally, since the DFL is
>>>> typically used in a FPGA, the broken hardware, can and should be fixed
>>>> in most cases.
>>> How is lspci supposed to work ?
>> There is an example for one card using IOFS and DFL.
>>
>> # lspci | grep acc
>> b1:00.0 Processing accelerators: Intel Corporation Device bcce (rev 01)
>> b1:00.1 Processing accelerators: Intel Corporation Device bcce
>> b1:00.2 Processing accelerators: Intel Corporation Device bcce
>> b1:00.3 Processing accelerators: Red Hat, Inc. Virtio network device
>> b1:00.4 Processing accelerators: Intel Corporation Device bcce
>>
>> Note: There 5 PFs in this card, it exports the management functions via
>> PF0(b1:00.0),
>> Other PFs like b1:00.1, b1:00.2, b1:00.4, are using for testing, which
>> depends on RTL designer
>> or project requirement. The PF3 instance a VirtIO net device for example,
>> will bind with virtio-net driver
>> presenting itself as a network interface to the OS.
Hi Tom,
These are very good questions, and the answers will be addressed in the
documentation associated with a v2 submission of this patch.
>
> What I mean there is heterogeneous set of cards in one machine, how do you
> tell which card is which ?
If the PCI PID/VID is generic, indicating only that there is one or more
DFL, then some other mechanism must be used to differentiate the cards.
One could use unique PCI sub-PID/sub-VIDs to differentiate specific
implementations. One could also use some register in BAR space to help
identify the card, or one could use PCI Vital Product Data (VPD) to
provide detailed information about the running FPGA design on the card.
>
> Or in a datacenter where the machines are all remote and admin has to flash
> just the n6000's ?
This problem exists with the N3000 cards. Depending on the FPGA
configuration, the line side of the card could be very different (e.g.
4x10Gb or 2x2x25Gb). The network operator must make sure to update a
particular N3000 card with the correct FPGA image type. In the case of
the N3000 there is a register exposed through sysfs containing the
"Bitstream ID" which contains the line side configuration of the FPGA.
>
> How could she find just the n6000's with lspci ?
If you only wanted to use lspci to determine the card, then
differentiating PCI sub-VID/sub-PID could be used or VPD could be used.
>
> How would the driver know ?
The dfl-pci driver is fairly generic in that it doesn't really care about
the PCI PID/VID because all it really does enumerate the DFLs. It is the
individual dfl drivers that may need to know hw differences/bugs for that
component IP.
>
> Tom
>
>>
>>> A dfl set can change with fw updates and in theory different boards could
>>> have
>>> the same set.
>>>
>>> Tom
>>>
>>>> Matthew
>>>>>>> Tom
>>>>>>>
>>>>>>>> /* VF Device */
>>>>>>>> #define PCIE_DEVICE_ID_VF_INT_5_X 0xBCBF
>>>>>>>> #define PCIE_DEVICE_ID_VF_INT_6_X 0xBCC1
>>>>>>>> #define PCIE_DEVICE_ID_VF_DSC_1_X 0x09C5
>>>>>>>> #define PCIE_DEVICE_ID_INTEL_PAC_D5005_VF 0x0B2C
>>>>>>>> +#define PCIE_DEVICE_ID_INTEL_OFS_VF 0xbccf
>>>>>>>>
>>>>>>>> static struct pci_device_id cci_pcie_id_tbl[] = {
>>>>>>>> {PCI_DEVICE(PCI_VENDOR_ID_INTEL,
>>>>>>>> PCIE_DEVICE_ID_PF_INT_5_X),},
>>>>>>> @@
>>>>>>>> -95,6 +97,8 @@ static struct pci_device_id cci_pcie_id_tbl[] = {
>>>>>>>> {PCI_DEVICE(PCI_VENDOR_ID_INTEL,
>>>>>>> PCIE_DEVICE_ID_INTEL_PAC_D5005_VF),},
>>>>>>>> {PCI_DEVICE(PCI_VENDOR_ID_SILICOM_DENMARK,
>>>>>>> PCIE_DEVICE_ID_SILICOM_PAC_N5010),},
>>>>>>>> {PCI_DEVICE(PCI_VENDOR_ID_SILICOM_DENMARK,
>>>>>>>> PCIE_DEVICE_ID_SILICOM_PAC_N5011),},
>>>>>>>> + {PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCIE_DEVICE_ID_INTEL_OFS),},
>>>>>>>> + {PCI_DEVICE(PCI_VENDOR_ID_INTEL,
>>>>>>> PCIE_DEVICE_ID_INTEL_OFS_VF),},
>>>>>>>> {0,}
>>>>>>>> };
>>>>>>>> MODULE_DEVICE_TABLE(pci, cci_pcie_id_tbl);
>>>>>
>
>
^ permalink raw reply [flat|nested] 47+ messages in thread* RE: [PATCH v1 7/7] fpga: dfl: pci: Add generic OFS PCI PID
2022-02-24 17:54 ` matthew.gerlach
@ 2022-02-28 10:57 ` Wu, Hao
2022-03-01 0:25 ` matthew.gerlach
0 siblings, 1 reply; 47+ messages in thread
From: Wu, Hao @ 2022-02-28 10:57 UTC (permalink / raw)
To: matthew.gerlach, Tom Rix
Cc: Zhang, Tianfei, mdf, Xu, Yilun, linux-fpga, linux-doc,
linux-kernel, corbet, Grier, Aaron J
> >
> > On 2/21/22 7:11 PM, Zhang, Tianfei wrote:
> >>
> >>> -----Original Message-----
> >>> From: Tom Rix <trix@redhat.com>
> >>> Sent: Tuesday, February 22, 2022 2:10 AM
> >>> To: matthew.gerlach@linux.intel.com
> >>> Cc: 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; corbet@lwn.net
> >>> Subject: Re: [PATCH v1 7/7] fpga: dfl: pci: Add generic OFS PCI PID
> >>>
> >>>
> >>> On 2/21/22 9:50 AM, matthew.gerlach@linux.intel.com wrote:
> >>>>
> >>>> On Fri, 18 Feb 2022, Tom Rix wrote:
> >>>>
> >>>>> On 2/18/22 1:03 AM, Zhang, Tianfei wrote:
> >>>>>>> -----Original Message-----
> >>>>>>> From: Tom Rix <trix@redhat.com>
> >>>>>>> Sent: Wednesday, February 16, 2022 12:16 AM
> >>>>>>> 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 7/7] fpga: dfl: pci: Add generic OFS PCI PID
> >>>>>>>
> >>>>>>>
> >>>>>>> On 2/14/22 3:26 AM, Tianfei zhang wrote:
> >>>>>>>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> >>>>>>>>
> >>>>>>>> Add the PCI product id for an Open FPGA Stack PCI card.
> >>>>>>> Is there a URL to the card ?
> >>>>>> This PCIe Device IDs have registered by Intel.
> >>>>> A URL is useful to introduce the board, Is there one ?
> >>>>>>>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> >>>>>>>> Signed-off-by: Tianfei Zhang <tianfei.zhang@intel.com>
> >>>>>>>> ---
> >>>>>>>> drivers/fpga/dfl-pci.c | 4 ++++
> >>>>>>>> 1 file changed, 4 insertions(+)
> >>>>>>>>
> >>>>>>>> diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c index
> >>>>>>>> 83b604d6dbe6..cb2fbf3eb918 100644
> >>>>>>>> --- a/drivers/fpga/dfl-pci.c
> >>>>>>>> +++ b/drivers/fpga/dfl-pci.c
> >>>>>>>> @@ -76,12 +76,14 @@ static void cci_pci_free_irq(struct pci_dev
> >>>>>>>> *pcidev)
> >>>>>>>> #define PCIE_DEVICE_ID_INTEL_PAC_D5005 0x0B2B
> >>>>>>>> #define PCIE_DEVICE_ID_SILICOM_PAC_N5010 0x1000
> >>>>>>>> #define PCIE_DEVICE_ID_SILICOM_PAC_N5011 0x1001
> >>>>>>>> +#define PCIE_DEVICE_ID_INTEL_OFS 0xbcce
> >>>>>>> INTEL_OFS is a generic name, pci id's map to specific cards
> >>>>>>>
> >>>>>>> Is there a more specific name for this card ?
> >>>>>> I think using INTEL_OFS is better, because INTEL_OFS is the Generic
> >>>>>> development platform can support multiple cards which using OFS
> >>>>>> specification, like Intel PAC N6000 card.
> >>>>> I would prefer something like PCIE_DEVICE_ID_INTEL_PAC_N6000
> because
> >>>>> it follows an existing pattern. Make it easy on a developer, they
> >>>>> will look at their board or box, see X and try to find something
> >>>>> similar in the driver source.
> >>>>>
> >>>>> To use OSF_ * the name needs a suffix to differentiate it from future
> >>>>> cards that will also use ofs.
> >>>>>
> >>>>> If this really is a generic id please explain in the doc patch how
> >>>>> every future board with use this single id and how a driver could
> >>>>> work around a hw problem in a specific board with a pci id covering
> >>>>> multiple boards.
> >>>>>
> >>>>> Tom
> >>>> Hi Tom,
> >>>>
> >>>> The intent is to have a generic device id that can be used with many
> >>>> different boards. Currently, we have FPGA implementations for 3
> >>>> different boards using this generic id. We may need a better name for
> >>>> device id than OFS. More precisely this generic device id means a PCI
> >>>> function that is described by a Device Feature List (DFL). How about
> >>>> PCIE_DEVICE_ID_INTEL_DFL?
> >>>>
> >>>> With a DFL device id, the functionality of the PF/VF is determined by
> >>>> the contents of the DFL. Each Device Feature Header (DFH) in the DFL
> >>>> has a revision field that can be used identify "broken" hw, or new
> >>>> functionality added to a feature. Additionally, since the DFL is
> >>>> typically used in a FPGA, the broken hardware, can and should be fixed
> >>>> in most cases.
> >>> How is lspci supposed to work ?
> >> There is an example for one card using IOFS and DFL.
> >>
> >> # lspci | grep acc
> >> b1:00.0 Processing accelerators: Intel Corporation Device bcce (rev 01)
> >> b1:00.1 Processing accelerators: Intel Corporation Device bcce
> >> b1:00.2 Processing accelerators: Intel Corporation Device bcce
> >> b1:00.3 Processing accelerators: Red Hat, Inc. Virtio network device
> >> b1:00.4 Processing accelerators: Intel Corporation Device bcce
> >>
> >> Note: There 5 PFs in this card, it exports the management functions via
> >> PF0(b1:00.0),
> >> Other PFs like b1:00.1, b1:00.2, b1:00.4, are using for testing, which
> >> depends on RTL designer
> >> or project requirement. The PF3 instance a VirtIO net device for example,
> >> will bind with virtio-net driver
> >> presenting itself as a network interface to the OS.
>
> Hi Tom,
>
> These are very good questions, and the answers will be addressed in the
> documentation associated with a v2 submission of this patch.
>
> >
> > What I mean there is heterogeneous set of cards in one machine, how do you
> > tell which card is which ?
>
> If the PCI PID/VID is generic, indicating only that there is one or more
> DFL, then some other mechanism must be used to differentiate the cards.
> One could use unique PCI sub-PID/sub-VIDs to differentiate specific
> implementations. One could also use some register in BAR space to help
> identify the card, or one could use PCI Vital Product Data (VPD) to
> provide detailed information about the running FPGA design on the card.
Ideally DFL has different scope than PCI. DFL is a higher layer concept than
PCI, as DFL can be applied to PCI device, platform device or even other devices.
If some PCI level quirks need to be applied before accessing BAR for one card,
then DFL may not be able to help at all. Use PCI level solution should be better,
and different VID/DID may be the easiest solution.
Hao
>
> >
> > Or in a datacenter where the machines are all remote and admin has to flash
> > just the n6000's ?
>
> This problem exists with the N3000 cards. Depending on the FPGA
> configuration, the line side of the card could be very different (e.g.
> 4x10Gb or 2x2x25Gb). The network operator must make sure to update a
> particular N3000 card with the correct FPGA image type. In the case of
> the N3000 there is a register exposed through sysfs containing the
> "Bitstream ID" which contains the line side configuration of the FPGA.
>
> >
> > How could she find just the n6000's with lspci ?
>
> If you only wanted to use lspci to determine the card, then
> differentiating PCI sub-VID/sub-PID could be used or VPD could be used.
>
> >
> > How would the driver know ?
>
> The dfl-pci driver is fairly generic in that it doesn't really care about
> the PCI PID/VID because all it really does enumerate the DFLs. It is the
> individual dfl drivers that may need to know hw differences/bugs for that
> component IP.
>
> >
> > Tom
> >
> >>
> >>> A dfl set can change with fw updates and in theory different boards could
> >>> have
> >>> the same set.
> >>>
> >>> Tom
> >>>
> >>>> Matthew
> >>>>>>> Tom
> >>>>>>>
> >>>>>>>> /* VF Device */
> >>>>>>>> #define PCIE_DEVICE_ID_VF_INT_5_X 0xBCBF
> >>>>>>>> #define PCIE_DEVICE_ID_VF_INT_6_X 0xBCC1
> >>>>>>>> #define PCIE_DEVICE_ID_VF_DSC_1_X 0x09C5
> >>>>>>>> #define PCIE_DEVICE_ID_INTEL_PAC_D5005_VF 0x0B2C
> >>>>>>>> +#define PCIE_DEVICE_ID_INTEL_OFS_VF 0xbccf
> >>>>>>>>
> >>>>>>>> static struct pci_device_id cci_pcie_id_tbl[] = {
> >>>>>>>> {PCI_DEVICE(PCI_VENDOR_ID_INTEL,
> >>>>>>>> PCIE_DEVICE_ID_PF_INT_5_X),},
> >>>>>>> @@
> >>>>>>>> -95,6 +97,8 @@ static struct pci_device_id cci_pcie_id_tbl[] = {
> >>>>>>>> {PCI_DEVICE(PCI_VENDOR_ID_INTEL,
> >>>>>>> PCIE_DEVICE_ID_INTEL_PAC_D5005_VF),},
> >>>>>>>> {PCI_DEVICE(PCI_VENDOR_ID_SILICOM_DENMARK,
> >>>>>>> PCIE_DEVICE_ID_SILICOM_PAC_N5010),},
> >>>>>>>> {PCI_DEVICE(PCI_VENDOR_ID_SILICOM_DENMARK,
> >>>>>>>> PCIE_DEVICE_ID_SILICOM_PAC_N5011),},
> >>>>>>>> + {PCI_DEVICE(PCI_VENDOR_ID_INTEL,
> PCIE_DEVICE_ID_INTEL_OFS),},
> >>>>>>>> + {PCI_DEVICE(PCI_VENDOR_ID_INTEL,
> >>>>>>> PCIE_DEVICE_ID_INTEL_OFS_VF),},
> >>>>>>>> {0,}
> >>>>>>>> };
> >>>>>>>> MODULE_DEVICE_TABLE(pci, cci_pcie_id_tbl);
> >>>>>
> >
> >
^ permalink raw reply [flat|nested] 47+ messages in thread* RE: [PATCH v1 7/7] fpga: dfl: pci: Add generic OFS PCI PID
2022-02-28 10:57 ` Wu, Hao
@ 2022-03-01 0:25 ` matthew.gerlach
0 siblings, 0 replies; 47+ messages in thread
From: matthew.gerlach @ 2022-03-01 0:25 UTC (permalink / raw)
To: Wu, Hao
Cc: Tom Rix, Zhang, Tianfei, mdf, Xu, Yilun, linux-fpga, linux-doc,
linux-kernel, corbet, Grier, Aaron J
[-- Attachment #1: Type: text/plain, Size: 9132 bytes --]
On Mon, 28 Feb 2022, Wu, Hao wrote:
>>>
>>> On 2/21/22 7:11 PM, Zhang, Tianfei wrote:
>>>>
>>>>> -----Original Message-----
>>>>> From: Tom Rix <trix@redhat.com>
>>>>> Sent: Tuesday, February 22, 2022 2:10 AM
>>>>> To: matthew.gerlach@linux.intel.com
>>>>> Cc: 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; corbet@lwn.net
>>>>> Subject: Re: [PATCH v1 7/7] fpga: dfl: pci: Add generic OFS PCI PID
>>>>>
>>>>>
>>>>> On 2/21/22 9:50 AM, matthew.gerlach@linux.intel.com wrote:
>>>>>>
>>>>>> On Fri, 18 Feb 2022, Tom Rix wrote:
>>>>>>
>>>>>>> On 2/18/22 1:03 AM, Zhang, Tianfei wrote:
>>>>>>>>> -----Original Message-----
>>>>>>>>> From: Tom Rix <trix@redhat.com>
>>>>>>>>> Sent: Wednesday, February 16, 2022 12:16 AM
>>>>>>>>> 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 7/7] fpga: dfl: pci: Add generic OFS PCI PID
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 2/14/22 3:26 AM, Tianfei zhang wrote:
>>>>>>>>>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>>>>>>>>>
>>>>>>>>>> Add the PCI product id for an Open FPGA Stack PCI card.
>>>>>>>>> Is there a URL to the card ?
>>>>>>>> This PCIe Device IDs have registered by Intel.
>>>>>>> A URL is useful to introduce the board, Is there one ?
>>>>>>>>>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>>>>>>>>> Signed-off-by: Tianfei Zhang <tianfei.zhang@intel.com>
>>>>>>>>>> ---
>>>>>>>>>> drivers/fpga/dfl-pci.c | 4 ++++
>>>>>>>>>> 1 file changed, 4 insertions(+)
>>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c index
>>>>>>>>>> 83b604d6dbe6..cb2fbf3eb918 100644
>>>>>>>>>> --- a/drivers/fpga/dfl-pci.c
>>>>>>>>>> +++ b/drivers/fpga/dfl-pci.c
>>>>>>>>>> @@ -76,12 +76,14 @@ static void cci_pci_free_irq(struct pci_dev
>>>>>>>>>> *pcidev)
>>>>>>>>>> #define PCIE_DEVICE_ID_INTEL_PAC_D5005 0x0B2B
>>>>>>>>>> #define PCIE_DEVICE_ID_SILICOM_PAC_N5010 0x1000
>>>>>>>>>> #define PCIE_DEVICE_ID_SILICOM_PAC_N5011 0x1001
>>>>>>>>>> +#define PCIE_DEVICE_ID_INTEL_OFS 0xbcce
>>>>>>>>> INTEL_OFS is a generic name, pci id's map to specific cards
>>>>>>>>>
>>>>>>>>> Is there a more specific name for this card ?
>>>>>>>> I think using INTEL_OFS is better, because INTEL_OFS is the Generic
>>>>>>>> development platform can support multiple cards which using OFS
>>>>>>>> specification, like Intel PAC N6000 card.
>>>>>>> I would prefer something like PCIE_DEVICE_ID_INTEL_PAC_N6000
>> because
>>>>>>> it follows an existing pattern. Make it easy on a developer, they
>>>>>>> will look at their board or box, see X and try to find something
>>>>>>> similar in the driver source.
>>>>>>>
>>>>>>> To use OSF_ * the name needs a suffix to differentiate it from future
>>>>>>> cards that will also use ofs.
>>>>>>>
>>>>>>> If this really is a generic id please explain in the doc patch how
>>>>>>> every future board with use this single id and how a driver could
>>>>>>> work around a hw problem in a specific board with a pci id covering
>>>>>>> multiple boards.
>>>>>>>
>>>>>>> Tom
>>>>>> Hi Tom,
>>>>>>
>>>>>> The intent is to have a generic device id that can be used with many
>>>>>> different boards. Currently, we have FPGA implementations for 3
>>>>>> different boards using this generic id. We may need a better name for
>>>>>> device id than OFS. More precisely this generic device id means a PCI
>>>>>> function that is described by a Device Feature List (DFL). How about
>>>>>> PCIE_DEVICE_ID_INTEL_DFL?
>>>>>>
>>>>>> With a DFL device id, the functionality of the PF/VF is determined by
>>>>>> the contents of the DFL. Each Device Feature Header (DFH) in the DFL
>>>>>> has a revision field that can be used identify "broken" hw, or new
>>>>>> functionality added to a feature. Additionally, since the DFL is
>>>>>> typically used in a FPGA, the broken hardware, can and should be fixed
>>>>>> in most cases.
>>>>> How is lspci supposed to work ?
>>>> There is an example for one card using IOFS and DFL.
>>>>
>>>> # lspci | grep acc
>>>> b1:00.0 Processing accelerators: Intel Corporation Device bcce (rev 01)
>>>> b1:00.1 Processing accelerators: Intel Corporation Device bcce
>>>> b1:00.2 Processing accelerators: Intel Corporation Device bcce
>>>> b1:00.3 Processing accelerators: Red Hat, Inc. Virtio network device
>>>> b1:00.4 Processing accelerators: Intel Corporation Device bcce
>>>>
>>>> Note: There 5 PFs in this card, it exports the management functions via
>>>> PF0(b1:00.0),
>>>> Other PFs like b1:00.1, b1:00.2, b1:00.4, are using for testing, which
>>>> depends on RTL designer
>>>> or project requirement. The PF3 instance a VirtIO net device for example,
>>>> will bind with virtio-net driver
>>>> presenting itself as a network interface to the OS.
>>
>> Hi Tom,
>>
>> These are very good questions, and the answers will be addressed in the
>> documentation associated with a v2 submission of this patch.
>>
>>>
>>> What I mean there is heterogeneous set of cards in one machine, how do you
>>> tell which card is which ?
>>
>> If the PCI PID/VID is generic, indicating only that there is one or more
>> DFL, then some other mechanism must be used to differentiate the cards.
>> One could use unique PCI sub-PID/sub-VIDs to differentiate specific
>> implementations. One could also use some register in BAR space to help
>> identify the card, or one could use PCI Vital Product Data (VPD) to
>> provide detailed information about the running FPGA design on the card.
>
> Ideally DFL has different scope than PCI. DFL is a higher layer concept than
> PCI, as DFL can be applied to PCI device, platform device or even other devices.
> If some PCI level quirks need to be applied before accessing BAR for one card,
> then DFL may not be able to help at all. Use PCI level solution should be better,
> and different VID/DID may be the easiest solution.
>
> Hao
Very good point Hao. DFL is a higher layer than PCI. So PCI level quirks
would need to be handled at the PCI level. The VID/DID, optionally
in conjuction with the Subsytem Vendor ID and Substem ID, would be used to
determine how the quirks were applied.
Matthew
>>
>>>
>>> Or in a datacenter where the machines are all remote and admin has to flash
>>> just the n6000's ?
>>
>> This problem exists with the N3000 cards. Depending on the FPGA
>> configuration, the line side of the card could be very different (e.g.
>> 4x10Gb or 2x2x25Gb). The network operator must make sure to update a
>> particular N3000 card with the correct FPGA image type. In the case of
>> the N3000 there is a register exposed through sysfs containing the
>> "Bitstream ID" which contains the line side configuration of the FPGA.
>>
>>>
>>> How could she find just the n6000's with lspci ?
>>
>> If you only wanted to use lspci to determine the card, then
>> differentiating PCI sub-VID/sub-PID could be used or VPD could be used.
>>
>>>
>>> How would the driver know ?
>>
>> The dfl-pci driver is fairly generic in that it doesn't really care about
>> the PCI PID/VID because all it really does enumerate the DFLs. It is the
>> individual dfl drivers that may need to know hw differences/bugs for that
>> component IP.
>>
>>>
>>> Tom
>>>
>>>>
>>>>> A dfl set can change with fw updates and in theory different boards could
>>>>> have
>>>>> the same set.
>>>>>
>>>>> Tom
>>>>>
>>>>>> Matthew
>>>>>>>>> Tom
>>>>>>>>>
>>>>>>>>>> /* VF Device */
>>>>>>>>>> #define PCIE_DEVICE_ID_VF_INT_5_X 0xBCBF
>>>>>>>>>> #define PCIE_DEVICE_ID_VF_INT_6_X 0xBCC1
>>>>>>>>>> #define PCIE_DEVICE_ID_VF_DSC_1_X 0x09C5
>>>>>>>>>> #define PCIE_DEVICE_ID_INTEL_PAC_D5005_VF 0x0B2C
>>>>>>>>>> +#define PCIE_DEVICE_ID_INTEL_OFS_VF 0xbccf
>>>>>>>>>>
>>>>>>>>>> static struct pci_device_id cci_pcie_id_tbl[] = {
>>>>>>>>>> {PCI_DEVICE(PCI_VENDOR_ID_INTEL,
>>>>>>>>>> PCIE_DEVICE_ID_PF_INT_5_X),},
>>>>>>>>> @@
>>>>>>>>>> -95,6 +97,8 @@ static struct pci_device_id cci_pcie_id_tbl[] = {
>>>>>>>>>> {PCI_DEVICE(PCI_VENDOR_ID_INTEL,
>>>>>>>>> PCIE_DEVICE_ID_INTEL_PAC_D5005_VF),},
>>>>>>>>>> {PCI_DEVICE(PCI_VENDOR_ID_SILICOM_DENMARK,
>>>>>>>>> PCIE_DEVICE_ID_SILICOM_PAC_N5010),},
>>>>>>>>>> {PCI_DEVICE(PCI_VENDOR_ID_SILICOM_DENMARK,
>>>>>>>>>> PCIE_DEVICE_ID_SILICOM_PAC_N5011),},
>>>>>>>>>> + {PCI_DEVICE(PCI_VENDOR_ID_INTEL,
>> PCIE_DEVICE_ID_INTEL_OFS),},
>>>>>>>>>> + {PCI_DEVICE(PCI_VENDOR_ID_INTEL,
>>>>>>>>> PCIE_DEVICE_ID_INTEL_OFS_VF),},
>>>>>>>>>> {0,}
>>>>>>>>>> };
>>>>>>>>>> MODULE_DEVICE_TABLE(pci, cci_pcie_id_tbl);
>>>>>>>
>>>
>>>
>
^ permalink raw reply [flat|nested] 47+ messages in thread