> -----Original Message-----
> From: Ye, Xiaolong
> Sent: Wednesday, September 25, 2019 12:02 AM
> To: Pei, Andy <andy....@intel.com>
> Cc: dev@dpdk.org; Xu, Rosen <rosen...@intel.com>; Zhang, Tianfei
> <tianfei.zh...@intel.com>; Zhang, Qi Z <qi.z.zh...@intel.com>; Lomartire,
> David <david.lomart...@intel.com>; Yigit, Ferruh <ferruh.yi...@intel.com>
> Subject: Re: [PATCH v6 02/17] raw/ifpga/base: add irq support
>
> On 09/19, Andy Pei wrote:
> >From: Tianfei zhang <tianfei.zh...@intel.com>
> >
> >Add irq support for ifpga FME globle error, port error and uint unit.
>
> s/globle/global
Thanks, will fix in next version.
>
> >We implmented this feature by vfio interrupt mechanism.
> >
> >Signed-off-by: Tianfei zhang <tianfei.zh...@intel.com>
> >Signed-off-by: Andy Pei <andy....@intel.com>
> >---
> > drivers/raw/ifpga/base/ifpga_feature_dev.c | 60
> ++++++++++++++++++++++++++++++
> > drivers/raw/ifpga/base/ifpga_fme_error.c | 22 +++++++++++
> > drivers/raw/ifpga/base/ifpga_port.c | 20 ++++++++++
> > drivers/raw/ifpga/base/ifpga_port_error.c | 21 +++++++++++
> > 4 files changed, 123 insertions(+)
> >
> >diff --git a/drivers/raw/ifpga/base/ifpga_feature_dev.c
> >b/drivers/raw/ifpga/base/ifpga_feature_dev.c
> >index 63c8bcc..f0fb242 100644
> >--- a/drivers/raw/ifpga/base/ifpga_feature_dev.c
> >+++ b/drivers/raw/ifpga/base/ifpga_feature_dev.c
> >@@ -3,6 +3,7 @@
> > */
> >
> > #include <sys/ioctl.h>
> >+#include <rte_vfio.h>
> >
> > #include "ifpga_feature_dev.h"
> >
> >@@ -331,3 +332,62 @@ int port_hw_init(struct ifpga_port_hw *port)
> > port_hw_uinit(port);
> > return ret;
> > }
> >+
> >+/*
> >+ * FIXME: we should get msix vec count during pci enumeration instead
> >+of
> >+ * below hardcode value.
> >+ */
> >+#define FPGA_MSIX_VEC_COUNT 20
>
> So what is preventing us from getting msix vec count from pci enumeration?
>
> >+/* irq set buffer length for interrupt */ #define MSIX_IRQ_SET_BUF_LEN
> >+(sizeof(struct vfio_irq_set) + \
> >+ sizeof(int) * FPGA_MSIX_VEC_COUNT)
> >+
> >+/* only support msix for now*/
> >+static int vfio_msix_enable_block(s32 vfio_dev_fd, unsigned int vec_start,
> >+ unsigned int count, s32 *fds)
>
> DPDK convention is put the function return type in a separate line.
>
> >+{
> >+ char irq_set_buf[MSIX_IRQ_SET_BUF_LEN];
> >+ struct vfio_irq_set *irq_set;
> >+ int len, ret;
> >+ int *fd_ptr;
> >+
> >+ len = sizeof(irq_set_buf);
> >+
> >+ irq_set = (struct vfio_irq_set *)irq_set_buf;
> >+ irq_set->argsz = len;
> >+ irq_set->count = count;
> >+ irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD |
> >+ VFIO_IRQ_SET_ACTION_TRIGGER;
> >+ irq_set->index = VFIO_PCI_MSIX_IRQ_INDEX;
> >+ irq_set->start = vec_start;
> >+
> >+ fd_ptr = (int *)&irq_set->data;
> >+ opae_memcpy(fd_ptr, fds, sizeof(int) * count);
> >+
> >+ ret = ioctl(vfio_dev_fd, VFIO_DEVICE_SET_IRQS, irq_set);
> >+ if (ret)
> >+ printf("Error enabling MSI-X interrupts\n");
> >+
> >+ return ret;
> >+}
> >+
> >+int fpga_msix_set_block(struct ifpga_feature *feature, unsigned int start,
> >+ unsigned int count, s32 *fds)
>
> Ditto.
>
> >+{
> >+ struct feature_irq_ctx *ctx = feature->ctx;
> >+ unsigned int i;
> >+ int ret;
> >+
> >+ if (start >= feature->ctx_num || start + count > feature->ctx_num)
> >+ return -EINVAL;
> >+
> >+ /* assume that each feature has continuous vector space in msix*/
> >+ ret = vfio_msix_enable_block(feature->vfio_dev_fd,
> >+ ctx[start].idx, count, fds);
> >+ if (!ret) {
> >+ for (i = 0; i < count; i++)
> >+ ctx[i].eventfd = fds[i];
> >+ }
> >+
> >+ return ret;
> >+}
> >diff --git a/drivers/raw/ifpga/base/ifpga_fme_error.c
> >b/drivers/raw/ifpga/base/ifpga_fme_error.c
> >index 3794564..068f52c 100644
> >--- a/drivers/raw/ifpga/base/ifpga_fme_error.c
> >+++ b/drivers/raw/ifpga/base/ifpga_fme_error.c
> >@@ -373,9 +373,31 @@ static int fme_global_error_set_prop(struct
> ifpga_feature *feature,
> > return -ENOENT;
> > }
> >
> >+static int fme_global_err_set_irq(struct ifpga_feature *feature, void
> >+*irq_set)
>
> Ditto.
>
> >+{
> >+ struct fpga_fme_err_irq_set *err_irq_set =
> >+ (struct fpga_fme_err_irq_set *)irq_set;
>
> Cast is not needed for void *.
Will fix in next version.
>
> >+ struct ifpga_fme_hw *fme;
> >+ int ret;
> >+
> >+ fme = (struct ifpga_fme_hw *)feature->parent;
>
> Ditto.
>
> >+
> >+ spinlock_lock(&fme->lock);
> >+ if (!(fme->capability & FPGA_FME_CAP_ERR_IRQ)) {
> >+ spinlock_unlock(&fme->lock);
> >+ return -ENODEV;
> >+ }
> >+
> >+ ret = fpga_msix_set_block(feature, 0, 1, &err_irq_set->evtfd);
> >+ spinlock_unlock(&fme->lock);
> >+
> >+ return ret;
> >+}
> >+
> > struct ifpga_feature_ops fme_global_err_ops = {
> > .init = fme_global_error_init,
> > .uinit = fme_global_error_uinit,
> > .get_prop = fme_global_error_get_prop,
> > .set_prop = fme_global_error_set_prop,
> >+ .set_irq = fme_global_err_set_irq,
> > };
> >diff --git a/drivers/raw/ifpga/base/ifpga_port.c
> >b/drivers/raw/ifpga/base/ifpga_port.c
> >index 6c41164..56b04a6 100644
> >--- a/drivers/raw/ifpga/base/ifpga_port.c
> >+++ b/drivers/raw/ifpga/base/ifpga_port.c
> >@@ -384,9 +384,29 @@ static void port_uint_uinit(struct ifpga_feature
> *feature)
> > dev_info(NULL, "PORT UINT UInit.\n"); }
> >
> >+static int port_uint_set_irq(struct ifpga_feature *feature, void
> >+*irq_set)
>
> Ditto.
>
> >+{
> >+ struct fpga_uafu_irq_set *uafu_irq_set = irq_set;
> >+ struct ifpga_port_hw *port = feature->parent;
> >+ int ret;
> >+
> >+ spinlock_lock(&port->lock);
> >+ if (!(port->capability & FPGA_PORT_CAP_UAFU_IRQ)) {
> >+ spinlock_unlock(&port->lock);
> >+ return -ENODEV;
> >+ }
> >+
> >+ ret = fpga_msix_set_block(feature, uafu_irq_set->start,
> >+ uafu_irq_set->count, uafu_irq_set->evtfds);
> >+ spinlock_unlock(&port->lock);
> >+
> >+ return ret;
> >+}
> >+
> > struct ifpga_feature_ops ifpga_rawdev_port_uint_ops = {
> > .init = port_uint_init,
> > .uinit = port_uint_uinit,
> >+ .set_irq = port_uint_set_irq,
> > };
> >
> > static int port_afu_init(struct ifpga_feature *feature) diff --git
> >a/drivers/raw/ifpga/base/ifpga_port_error.c
> >b/drivers/raw/ifpga/base/ifpga_port_error.c
> >index 138284e..8aef7d7 100644
> >--- a/drivers/raw/ifpga/base/ifpga_port_error.c
> >+++ b/drivers/raw/ifpga/base/ifpga_port_error.c
> >@@ -136,9 +136,30 @@ static int port_error_set_prop(struct ifpga_feature
> *feature,
> > return -ENOENT;
> > }
> >
> >+static int port_error_set_irq(struct ifpga_feature *feature, void
> >+*irq_set)
>
> Ditto.
>
> >+{
> >+ struct fpga_port_err_irq_set *err_irq_set = irq_set;
> >+ struct ifpga_port_hw *port;
> >+ int ret;
> >+
> >+ port = feature->parent;
> >+
> >+ spinlock_lock(&port->lock);
> >+ if (!(port->capability & FPGA_PORT_CAP_ERR_IRQ)) {
> >+ spinlock_unlock(&port->lock);
> >+ return -ENODEV;
> >+ }
> >+
> >+ ret = fpga_msix_set_block(feature, 0, 1, &err_irq_set->evtfd);
> >+ spinlock_unlock(&port->lock);
> >+
> >+ return ret;
> >+}
> >+
>
> Above 3 new functions have a lot of similarity, better to extract out common
> function to reduce duplication.
Good suggestion, will fix in next version.
>
> Thanks,
> Xiaolong
>
> > struct ifpga_feature_ops ifpga_rawdev_port_error_ops = {
> > .init = port_error_init,
> > .uinit = port_error_uinit,
> > .get_prop = port_error_get_prop,
> > .set_prop = port_error_set_prop,
> >+ .set_irq = port_error_set_irq,
> > };
> >--
> >1.8.3.1
> >
