lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Tue, 4 Apr 2017 14:05:16 +0800
From:   Wu Hao <hao.wu@...el.com>
To:     Alan Tull <atull@...nel.org>
Cc:     Moritz Fischer <moritz.fischer@...us.com>,
        linux-fpga@...r.kernel.org,
        linux-kernel <linux-kernel@...r.kernel.org>,
        luwei.kang@...el.com, yi.z.zhang@...el.com,
        Tim Whisonant <tim.whisonant@...el.com>,
        Enno Luebbers <enno.luebbers@...el.com>,
        Shiva Rao <shiva.rao@...el.com>,
        Christopher Rauer <christopher.rauer@...el.com>,
        Alan Tull <alan.tull@...el.com>,
        Xiao Guangrong <guangrong.xiao@...ux.intel.com>
Subject: Re: [PATCH 11/16] fpga: intel: fme: add partial reconfiguration sub
 feature support

On Mon, Apr 03, 2017 at 11:30:55AM -0500, Alan Tull wrote:
> On Sat, Apr 1, 2017 at 6:08 AM, Wu Hao <hao.wu@...el.com> wrote:
> > On Fri, Mar 31, 2017 at 02:10:12PM -0500, Alan Tull wrote:
> >> On Thu, Mar 30, 2017 at 7:08 AM, Wu Hao <hao.wu@...el.com> wrote:
> >> > From: Kang Luwei <luwei.kang@...el.com>
> >> >
> >> > Partial Reconfiguration (PR) is the most important function for FME. It
> >> > allows reconfiguration for given Port/Accelerated Function Unit (AFU).
> >> >
> >> > This patch adds support for PR sub feature. In this patch, it registers
> >> > a fpga_mgr and implements fpga_manager_ops, and invoke fpga_mgr_buf_load
> >> > for PR operation once PR request received via ioctl. Below user space
> >> > interfaces are exposed by this sub feature.
> >> >
> >> > Sysfs interface:
> >> > * /sys/class/fpga/<fpga.x>/<intel-fpga-fme.x>/interface_id
> >> >   Read-only. Indicate the hardware interface information. Userspace
> >> >   applications need to check this interface to select correct green
> >> >   bitstream format before PR.
> >> >
> >> > Ioctl interface:
> >> > * FPGA_FME_PORT_PR
> >> >   Do partial reconfiguration per information from userspace, including
> >> >   target port(AFU), buffer size and address info. It returns the PR status
> >> >   (PR error code if failed) to userspace.
> >> >
> >> > Signed-off-by: Tim Whisonant <tim.whisonant@...el.com>
> >> > Signed-off-by: Enno Luebbers <enno.luebbers@...el.com>
> >> > Signed-off-by: Shiva Rao <shiva.rao@...el.com>
> >> > Signed-off-by: Christopher Rauer <christopher.rauer@...el.com>
> >> > Signed-off-by: Alan Tull <alan.tull@...el.com>
> >> > Signed-off-by: Kang Luwei <luwei.kang@...el.com>
> >> > Signed-off-by: Xiao Guangrong <guangrong.xiao@...ux.intel.com>
> >> > Signed-off-by: Wu Hao <hao.wu@...el.com>
> >> > ---
> >> >  drivers/fpga/intel/Makefile      |   2 +-
> >> >  drivers/fpga/intel/feature-dev.h |  58 ++++++
> >> >  drivers/fpga/intel/fme-main.c    |  44 ++++-
> >> >  drivers/fpga/intel/fme-pr.c      | 400 +++++++++++++++++++++++++++++++++++++++
> >> >  drivers/fpga/intel/fme.h         |  32 ++++
> >> >  include/uapi/linux/intel-fpga.h  |  44 +++++
> >> >  6 files changed, 578 insertions(+), 2 deletions(-)
> >> >  create mode 100644 drivers/fpga/intel/fme-pr.c
> >> >  create mode 100644 drivers/fpga/intel/fme.h
> >> >
> >> > diff --git a/drivers/fpga/intel/Makefile b/drivers/fpga/intel/Makefile
> >> > index 546861d..0452cb6 100644
> >> > --- a/drivers/fpga/intel/Makefile
> >> > +++ b/drivers/fpga/intel/Makefile
> >> > @@ -2,4 +2,4 @@ obj-$(CONFIG_INTEL_FPGA_PCI) += intel-fpga-pci.o
> >> >  obj-$(CONFIG_INTEL_FPGA_FME) += intel-fpga-fme.o
> >> >
> >> >  intel-fpga-pci-objs := pcie.o feature-dev.o
> >> > -intel-fpga-fme-objs := fme-main.o
> >> > +intel-fpga-fme-objs := fme-main.o fme-pr.o
> >> > diff --git a/drivers/fpga/intel/feature-dev.h b/drivers/fpga/intel/feature-dev.h
> >> > index dccc283..5a25c915 100644
> >> > --- a/drivers/fpga/intel/feature-dev.h
> >> > +++ b/drivers/fpga/intel/feature-dev.h
> >> > @@ -150,8 +150,66 @@ struct feature_fme_err {
> >> >  };
> >> >
> >> >  /* FME Partial Reconfiguration Sub Feature Register Set */
> >> > +/* FME PR Control Register */
> >> > +struct feature_fme_pr_ctl {
> >> > +       union {
> >> > +               u64 csr;
> >> > +               struct {
> >> > +                       u8  pr_reset:1;         /* Reset PR Engine */
> >> > +                       u8  rsvdz1:3;
> >> > +                       u8  pr_reset_ack:1;     /* Reset PR Engine Ack */
> >> > +                       u8  rsvdz2:3;
> >> > +                       u8  pr_regionid:2;      /* PR Region ID */
> >> > +                       u8  rsvdz3:2;
> >> > +                       u8  pr_start_req:1;     /* PR Start Request */
> >> > +                       u8  pr_push_complete:1; /* PR Data push complete */
> >> > +                       u8  pr_kind:1;          /* Load Customer or Intel GBS */
> >> > +                       u32 rsvdz4:17;
> >> > +                       u32 config_data;
> >> > +               };
> >> > +       };
> >> > +};
> >> > +
> >> > +/* FME PR Status Register */
> >> > +struct feature_fme_pr_status {
> >> > +       union {
> >> > +               u64 csr;
> >> > +               struct {
> >> > +                       u16 pr_credit:9;        /* Number of PR Credits */
> >> > +                       u8  rsvdz1:7;
> >> > +                       u8  pr_status:1;        /* PR Operation status */
> >> > +                       u8  rsvdz2:3;
> >> > +                       u8  pr_ctrlr_status:3;  /* Controller status */
> >> > +                       u8  rsvdz3:1;
> >> > +                       u8  pr_host_status:4;   /* PR Host status */
> >> > +                       u64 rsvdz4:36;
> >> > +               };
> >> > +       };
> >> > +};
> >> > +
> >> > +/* FME PR Data Register */
> >> > +struct feature_fme_pr_data {
> >> > +       union {
> >> > +               u64 csr;
> >> > +               struct {
> >> > +                       /* PR data from the raw-binary file */
> >> > +                       u32 pr_data_raw;
> >> > +                       u32 rsvd;
> >> > +               };
> >> > +       };
> >> > +};
> >> > +
> >> >  struct feature_fme_pr {
> >> >         struct feature_header header;
> >> > +       struct feature_fme_pr_ctl control;
> >> > +       struct feature_fme_pr_status status;
> >> > +       struct feature_fme_pr_data data;
> >> > +       u64 error;
> >> > +
> >> > +       u64 rsvd[16];
> >> > +
> >> > +       u64 intfc_id_l;         /* PR interface Id Low */
> >> > +       u64 intfc_id_h;         /* PR interface Id High */
> >> >  };
> >> >
> >> >  /* PORT Header Register Set */
> >> > diff --git a/drivers/fpga/intel/fme-main.c b/drivers/fpga/intel/fme-main.c
> >> > index 36d0c4c..0d9a7a6 100644
> >> > --- a/drivers/fpga/intel/fme-main.c
> >> > +++ b/drivers/fpga/intel/fme-main.c
> >> > @@ -23,6 +23,7 @@
> >> >  #include <linux/intel-fpga.h>
> >> >
> >> >  #include "feature-dev.h"
> >> > +#include "fme.h"
> >> >
> >> >  static ssize_t ports_num_show(struct device *dev,
> >> >                               struct device_attribute *attr, char *buf)
> >> > @@ -101,6 +102,10 @@ static struct feature_driver fme_feature_drvs[] = {
> >> >                 .ops = &fme_hdr_ops,
> >> >         },
> >> >         {
> >> > +               .name = FME_FEATURE_PR_MGMT,
> >> > +               .ops = &pr_mgmt_ops,
> >> > +       },
> >> > +       {
> >> >                 .ops = NULL,
> >> >         },
> >> >  };
> >> > @@ -182,14 +187,48 @@ static const struct file_operations fme_fops = {
> >> >         .unlocked_ioctl = fme_ioctl,
> >> >  };
> >> >
> >> > +static int fme_dev_init(struct platform_device *pdev)
> >> > +{
> >> > +       struct feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
> >> > +       struct fpga_fme *fme;
> >> > +
> >> > +       fme = devm_kzalloc(&pdev->dev, sizeof(*fme), GFP_KERNEL);
> >> > +       if (!fme)
> >> > +               return -ENOMEM;
> >> > +
> >> > +       fme->pdata = pdata;
> >> > +
> >> > +       mutex_lock(&pdata->lock);
> >> > +       fpga_pdata_set_private(pdata, fme);
> >> > +       mutex_unlock(&pdata->lock);
> >> > +       return 0;
> >> > +}
> >> > +
> >> > +static void fme_dev_destroy(struct platform_device *pdev)
> >> > +{
> >> > +       struct feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
> >> > +       struct fpga_fme *fme;
> >> > +
> >> > +       mutex_lock(&pdata->lock);
> >> > +       fme = fpga_pdata_get_private(pdata);
> >> > +       fpga_pdata_set_private(pdata, NULL);
> >> > +       mutex_unlock(&pdata->lock);
> >> > +
> >> > +       devm_kfree(&pdev->dev, fme);
> >> > +}
> >> > +
> >> >  static int fme_probe(struct platform_device *pdev)
> >> >  {
> >> >         int ret;
> >> >
> >> > -       ret = fpga_dev_feature_init(pdev, fme_feature_drvs);
> >> > +       ret = fme_dev_init(pdev);
> >> >         if (ret)
> >> >                 goto exit;
> >> >
> >> > +       ret = fpga_dev_feature_init(pdev, fme_feature_drvs);
> >> > +       if (ret)
> >> > +               goto dev_destroy;
> >> > +
> >> >         ret = fpga_register_dev_ops(pdev, &fme_fops, THIS_MODULE);
> >> >         if (ret)
> >> >                 goto feature_uinit;
> >> > @@ -198,6 +237,8 @@ static int fme_probe(struct platform_device *pdev)
> >> >
> >> >  feature_uinit:
> >> >         fpga_dev_feature_uinit(pdev);
> >> > +dev_destroy:
> >> > +       fme_dev_destroy(pdev);
> >> >  exit:
> >> >         return ret;
> >> >  }
> >> > @@ -206,6 +247,7 @@ static int fme_remove(struct platform_device *pdev)
> >> >  {
> >> >         fpga_dev_feature_uinit(pdev);
> >> >         fpga_unregister_dev_ops(pdev);
> >> > +       fme_dev_destroy(pdev);
> >> >         return 0;
> >> >  }
> >> >
> >> > diff --git a/drivers/fpga/intel/fme-pr.c b/drivers/fpga/intel/fme-pr.c
> >> > new file mode 100644
> >> > index 0000000..3b44a3e
> >> > --- /dev/null
> >> > +++ b/drivers/fpga/intel/fme-pr.c
> >> > @@ -0,0 +1,400 @@
> >> > +/*
> >> > + * Driver for Intel FPGA Management Engine (FME) Partial Reconfiguration
> >> > + *
> >> > + * Copyright (C) 2017 Intel Corporation, Inc.
> >> > + *
> >> > + * Authors:
> >> > + *   Kang Luwei <luwei.kang@...el.com>
> >> > + *   Xiao Guangrong <guangrong.xiao@...ux.intel.com>
> >> > + *   Joseph Grecco <joe.grecco@...el.com>
> >> > + *   Enno Luebbers <enno.luebbers@...el.com>
> >> > + *   Tim Whisonant <tim.whisonant@...el.com>
> >> > + *   Ananda Ravuri <ananda.ravuri@...el.com>
> >> > + *   Christopher Rauer <christopher.rauer@...el.com>
> >> > + *   Henry Mitchel <henry.mitchel@...el.com>
> >> > + *
> >> > + * This work is licensed under a dual BSD/GPLv2 license. When using or
> >> > + * redistributing this file, you may do so under either license. See the
> >> > + * LICENSE.BSD file under this directory for the BSD license and see
> >> > + * the COPYING file in the top-level directory for the GPLv2 license.
> >> > + */
> >> > +
> >> > +#include <linux/types.h>
> >> > +#include <linux/device.h>
> >> > +#include <linux/vmalloc.h>
> >> > +#include <linux/uaccess.h>
> >> > +#include <linux/fpga/fpga-mgr.h>
> >> > +#include <linux/intel-fpga.h>
> >> > +
> >> > +#include "feature-dev.h"
> >> > +#include "fme.h"
> >> > +
> >> > +#define PR_WAIT_TIMEOUT   8000000
> >> > +
> >> > +#define PR_HOST_STATUS_IDLE    0
> >> > +
> >> > +DEFINE_FPGA_PR_ERR_MSG(pr_err_msg);
> >> > +
> >> > +static ssize_t interface_id_show(struct device *dev,
> >> > +                                struct device_attribute *attr, char *buf)
> >> > +{
> >> > +       u64 intfc_id_l, intfc_id_h;
> >> > +       struct feature_fme_pr *fme_pr
> >> > +               = get_feature_ioaddr_by_index(dev, FME_FEATURE_ID_PR_MGMT);
> >> > +
> >> > +       intfc_id_l = readq(&fme_pr->intfc_id_l);
> >> > +       intfc_id_h = readq(&fme_pr->intfc_id_h);
> >> > +
> >> > +       return scnprintf(buf, PAGE_SIZE, "%016llx%016llx\n",
> >> > +                       (unsigned long long)intfc_id_h,
> >> > +                       (unsigned long long)intfc_id_l);
> >> > +}
> >> > +static DEVICE_ATTR_RO(interface_id);
> >> > +
> >> > +static struct attribute *pr_mgmt_attrs[] = {
> >> > +       &dev_attr_interface_id.attr,
> >> > +       NULL,
> >> > +};
> >> > +
> >> > +struct attribute_group pr_mgmt_attr_group = {
> >> > +       .attrs  = pr_mgmt_attrs,
> >> > +       .name   = "pr",
> >> > +};
> >> > +
> >> > +static u64
> >> > +pr_err_handle(struct platform_device *pdev, struct feature_fme_pr *fme_pr)
> >> > +{
> >> > +       struct feature_fme_pr_status fme_pr_status;
> >> > +       unsigned long err_code;
> >> > +       u64 fme_pr_error;
> >> > +       int i = 0;
> >> > +
> >> > +       fme_pr_status.csr = readq(&fme_pr->status);
> >> > +       if (!fme_pr_status.pr_status)
> >> > +               return 0;
> >> > +
> >> > +       err_code = fme_pr_error = readq(&fme_pr->error);
> >> > +       for_each_set_bit(i, &err_code, PR_MAX_ERR_NUM)
> >> > +               dev_dbg(&pdev->dev, "%s\n", pr_err_msg[i]);
> >> > +       writeq(fme_pr_error, &fme_pr->error);
> >> > +       return fme_pr_error;
> >> > +}
> >> > +
> >> > +static int fme_pr_write_init(struct fpga_manager *mgr,
> >> > +               struct fpga_image_info *info, const char *buf, size_t count)
> >> > +{
> >> > +       struct fpga_fme *fme = mgr->priv;
> >> > +       struct platform_device *pdev;
> >> > +       struct feature_fme_pr *fme_pr;
> >> > +       struct feature_fme_pr_ctl fme_pr_ctl;
> >> > +       struct feature_fme_pr_status fme_pr_status;
> >> > +
> >> > +       pdev = fme->pdata->dev;
> >> > +       fme_pr = get_feature_ioaddr_by_index(&pdev->dev,
> >> > +                               FME_FEATURE_ID_PR_MGMT);
> >> > +       if (!fme_pr)
> >> > +               return -EINVAL;
> >> > +
> >> > +       if (WARN_ON(info->flags != FPGA_MGR_PARTIAL_RECONFIG))
> >> > +               return -EINVAL;
> >> > +
> >> > +       dev_dbg(&pdev->dev, "resetting PR before initiated PR\n");
> >> > +
> >> > +       fme_pr_ctl.csr = readq(&fme_pr->control);
> >> > +       fme_pr_ctl.pr_reset = 1;
> >> > +       writeq(fme_pr_ctl.csr, &fme_pr->control);
> >> > +
> >> > +       fme_pr_ctl.pr_reset_ack = 1;
> >> > +
> >> > +       if (fpga_wait_register_field(pr_reset_ack, fme_pr_ctl,
> >> > +                                    &fme_pr->control, PR_WAIT_TIMEOUT, 1)) {
> >> > +               dev_err(&pdev->dev, "maximum PR timeout\n");
> >> > +               return -ETIMEDOUT;
> >> > +       }
> >> > +
> >> > +       fme_pr_ctl.csr = readq(&fme_pr->control);
> >> > +       fme_pr_ctl.pr_reset = 0;
> >> > +       writeq(fme_pr_ctl.csr, &fme_pr->control);
> >> > +
> >> > +       dev_dbg(&pdev->dev,
> >> > +               "waiting for PR resource in HW to be initialized and ready\n");
> >> > +
> >> > +       fme_pr_status.pr_host_status = PR_HOST_STATUS_IDLE;
> >> > +
> >> > +       if (fpga_wait_register_field(pr_host_status, fme_pr_status,
> >> > +                                    &fme_pr->status, PR_WAIT_TIMEOUT, 1)) {
> >> > +               dev_err(&pdev->dev, "maximum PR timeout\n");
> >> > +               return -ETIMEDOUT;
> >> > +       }
> >> > +
> >> > +       dev_dbg(&pdev->dev, "check if have any previous PR error\n");
> >> > +       pr_err_handle(pdev, fme_pr);
> >> > +       return 0;
> >> > +}
> >> > +
> >> > +static int fme_pr_write(struct fpga_manager *mgr,
> >> > +                       const char *buf, size_t count)
> >> > +{
> >> > +       struct fpga_fme *fme = mgr->priv;
> >> > +       struct platform_device *pdev;
> >> > +       struct feature_fme_pr *fme_pr;
> >> > +       struct feature_fme_pr_ctl fme_pr_ctl;
> >> > +       struct feature_fme_pr_status fme_pr_status;
> >> > +       struct feature_fme_pr_data fme_pr_data;
> >> > +       int delay, pr_credit, i = 0;
> >> > +
> >> > +       pdev = fme->pdata->dev;
> >> > +       fme_pr = get_feature_ioaddr_by_index(&pdev->dev,
> >> > +                               FME_FEATURE_ID_PR_MGMT);
> >> > +
> >> > +       dev_dbg(&pdev->dev, "set PR port ID and start request\n");
> >> > +
> >> > +       fme_pr_ctl.csr = readq(&fme_pr->control);
> >> > +       fme_pr_ctl.pr_regionid = fme->port_id;
> >> > +       fme_pr_ctl.pr_start_req = 1;
> >> > +       writeq(fme_pr_ctl.csr, &fme_pr->control);
> >> > +
> >> > +       dev_dbg(&pdev->dev, "pushing data from bitstream to HW\n");
> >> > +
> >> > +       fme_pr_status.csr = readq(&fme_pr->status);
> >> > +       pr_credit = fme_pr_status.pr_credit;
> >> > +
> >> > +       while (count > 0) {
> >> > +               delay = 0;
> >> > +               while (pr_credit <= 1) {
> >> > +                       if (delay++ > PR_WAIT_TIMEOUT) {
> >> > +                               dev_err(&pdev->dev, "maximum try\n");
> >> > +                               return -ETIMEDOUT;
> >> > +                       }
> >> > +                       udelay(1);
> >> > +
> >> > +                       fme_pr_status.csr = readq(&fme_pr->status);
> >> > +                       pr_credit = fme_pr_status.pr_credit;
> >> > +               };
> >> > +
> >> > +               if (count >= 4) {
> >> > +                       fme_pr_data.rsvd = 0;
> >> > +                       fme_pr_data.pr_data_raw = *(((u32 *)buf) + i);
> >> > +                       writeq(fme_pr_data.csr, &fme_pr->data);
> >> > +                       count -= 4;
> >> > +                       pr_credit--;
> >> > +                       i++;
> >> > +               } else {
> >> > +                       WARN_ON(1);
> >> > +                       return -EINVAL;
> >> > +               }
> >> > +       }
> >> > +
> >> > +       return 0;
> >> > +}
> >> > +
> >> > +static int fme_pr_write_complete(struct fpga_manager *mgr,
> >> > +                       struct fpga_image_info *info)
> >> > +{
> >> > +       struct fpga_fme *fme = mgr->priv;
> >> > +       struct platform_device *pdev;
> >> > +       struct feature_fme_pr *fme_pr;
> >> > +       struct feature_fme_pr_ctl fme_pr_ctl;
> >> > +
> >> > +       pdev = fme->pdata->dev;
> >> > +       fme_pr = get_feature_ioaddr_by_index(&pdev->dev,
> >> > +                               FME_FEATURE_ID_PR_MGMT);
> >> > +
> >> > +       fme_pr_ctl.csr = readq(&fme_pr->control);
> >> > +       fme_pr_ctl.pr_push_complete = 1;
> >> > +       writeq(fme_pr_ctl.csr, &fme_pr->control);
> >> > +
> >> > +       dev_dbg(&pdev->dev, "green bitstream push complete\n");
> >> > +       dev_dbg(&pdev->dev, "waiting for HW to release PR resource\n");
> >> > +
> >> > +       fme_pr_ctl.pr_start_req = 0;
> >> > +
> >> > +       if (fpga_wait_register_field(pr_start_req, fme_pr_ctl,
> >> > +                                    &fme_pr->control, PR_WAIT_TIMEOUT, 1)) {
> >> > +               dev_err(&pdev->dev, "maximum try.\n");
> >> > +               return -ETIMEDOUT;
> >> > +       }
> >> > +
> >> > +       dev_dbg(&pdev->dev, "PR operation complete, checking status\n");
> >> > +       fme->pr_err = pr_err_handle(pdev, fme_pr);
> >> > +       if (fme->pr_err)
> >> > +               return -EIO;
> >> > +
> >> > +       dev_dbg(&pdev->dev, "PR done successfully\n");
> >> > +       return 0;
> >> > +}
> >> > +
> >> > +static enum fpga_mgr_states fme_pr_state(struct fpga_manager *mgr)
> >> > +{
> >> > +       return FPGA_MGR_STATE_UNKNOWN;
> >> > +}
> >> > +
> >> > +static const struct fpga_manager_ops fme_pr_ops = {
> >> > +       .write_init = fme_pr_write_init,
> >> > +       .write = fme_pr_write,
> >> > +       .write_complete = fme_pr_write_complete,
> >> > +       .state = fme_pr_state,
> >> > +};
> >> > +
> >> > +static int fme_pr(struct platform_device *pdev, unsigned long arg)
> >> > +{
> >> > +       void __user *argp = (void __user *)arg;
> >> > +       struct feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
> >> > +       struct fpga_fme *fme;
> >> > +       struct fpga_manager *mgr;
> >> > +       struct feature_fme_header *fme_hdr;
> >> > +       struct feature_fme_capability fme_capability;
> >> > +       struct fpga_image_info info;
> >> > +       struct fpga_fme_port_pr port_pr;
> >> > +       struct platform_device *port;
> >> > +       unsigned long minsz;
> >> > +       void *buf = NULL;
> >> > +       int ret = 0;
> >> > +
> >> > +       minsz = offsetofend(struct fpga_fme_port_pr, status);
> >> > +
> >> > +       if (copy_from_user(&port_pr, argp, minsz))
> >> > +               return -EFAULT;
> >> > +
> >> > +       if (port_pr.argsz < minsz || port_pr.flags)
> >> > +               return -EINVAL;
> >> > +
> >> > +       if (!IS_ALIGNED(port_pr.buffer_size, 4))
> >> > +               return -EINVAL;
> >> > +
> >> > +       /* get fme header region */
> >> > +       fme_hdr = get_feature_ioaddr_by_index(&pdev->dev,
> >> > +                                       FME_FEATURE_ID_HEADER);
> >> > +       if (WARN_ON(!fme_hdr))
> >> > +               return -EINVAL;
> >> > +
> >> > +       /* check port id */
> >> > +       fme_capability.csr = readq(&fme_hdr->capability);
> >> > +       if (port_pr.port_id >= fme_capability.num_ports) {
> >> > +               dev_dbg(&pdev->dev, "port number more than maximum\n");
> >> > +               return -EINVAL;
> >> > +       }
> >> > +
> >> > +       if (!access_ok(VERIFY_READ, port_pr.buffer_address,
> >> > +                                   port_pr.buffer_size))
> >> > +               return -EFAULT;
> >> > +
> >> > +       buf = vmalloc(port_pr.buffer_size);
> >> > +       if (!buf)
> >> > +               return -ENOMEM;
> >> > +
> >> > +       if (copy_from_user(buf, (void __user *)port_pr.buffer_address,
> >> > +                                              port_pr.buffer_size)) {
> >> > +               ret = -EFAULT;
> >> > +               goto free_exit;
> >> > +       }
> >> > +
> >> > +       memset(&info, 0, sizeof(struct fpga_image_info));
> >> > +       info.flags = FPGA_MGR_PARTIAL_RECONFIG;
> >> > +
> >> > +       mgr = fpga_mgr_get(&pdev->dev);
> >> > +       if (IS_ERR(mgr)) {
> >> > +               ret = PTR_ERR(mgr);
> >> > +               goto free_exit;
> >> > +       }
> >> > +
> >> > +       mutex_lock(&pdata->lock);
> >> > +       fme = fpga_pdata_get_private(pdata);
> >> > +       /* fme device has been unregistered. */
> >> > +       if (!fme) {
> >> > +               ret = -EINVAL;
> >> > +               goto unlock_exit;
> >> > +       }
> >> > +
> >> > +       fme->pr_err = 0;
> >> > +       fme->port_id = port_pr.port_id;
> >>
> >> It looks like you're using private data to communicate with the
> >> driver, i.e. there is something you want to do with the fpga manager
> >> framework and it doesn't have that feature.  The better way would be
> >> for us to expand the framework so you don't need to do that.
> >>
> >> port_id is the kind of thing that should be communicated to the driver
> >> through fpga_image_info, so we could add that to the struct.  Should
> >> we call it port_id?
> 
> Let's call it region_id.
> 
> >> Or is there something more generic that may be
> >> useful in the future for other architectures?.
> >
> > Hi Alan
> >
> > Thanks for your feedback. :)
> >
> > As you know, each Intel FPGA device may have more than one accelerator,
> > and all accelerators share the same fpga_manager (only one FME module).
> > port_id = 0 means the first accelerator on this fpga devices. So it's
> > needed by the fpga_manager to distinguish one accelerator from others
> > for partial reconfiguration.
> >
> > Adding a member like a 'id' to fpga_image_info definitely works for us
> > in this case. We can add it this way, but I feel 'id' here seems not
> > related to fpga image, but characteristic of fpga region.
> 
> The  fpga_image_info struct started life as just image specific info,
> but I want it to go in the direction of including parameters needed to
> program it this specific time. Otherwise we are stuck having to keep
> adding parameters as our use of FPGA develops.  It probably could be
> documented better as 'information needed to program a FPGA image'
> rather than strictly 'information about this particular FPGA image'.
> My patch "fpga-mgr: pass parameters for loading fpga in image info"
> goes in this direction by having the buf, firmware name, or sg list
> passed in the info for the added fpga_mgr_load() function.  Actually I
> should probably simplify the API and get rid of fpga_mgr_buf_load,
> fpga_mgr_buf_load_sg, and fpga_mgr_firmware_load and require people to
> use fpga_mgr_load (passing all parameters in fpga_image_info).
>

Make sense.
 
> > It may be a
> > little confusing. One rough idea is that keep this info under fpga region
> > (maybe its private data), and pass the fpga-region to fpga_mgr_buf_load,
> 
> Yes, keep this info in fpga-region.  When the region wants to program
> using fpga-mgr, add the region id to fpga_image_info.  I propose
> calling it region_id.

Hm.. Do we need a function which moves info from region to image info?

Another idea is, add a priv to fpga_image_info, and use a common function
to pass the fpga_region's priv to fpga_image_info's priv before PR.
fpga-mgr then knows fpga_region priv info from the fpga_image_info.

> 
> > then fpga_manager knows the target region for partial reconfiguration.
> > If consider pr sysfs interface support under fpga-region in the future,
> > then we don't need to create a new 'id' sysfs file, as fpga-region itself
> > could provide this kind of info. But I'm not sure if this is the right
> > direction.
> >
> >>
> >> pr_err appears to be machine specific error codes that are
> >> communicated outside your low level driver.  (Calling it pr_err is
> >> extra confusing since Linux already has a commonly name function by
> >> the same name).  The framework has state, but that's not doing what
> >> you want here.  Maybe we could add a framework ops called status so
> >> that status could be communicated from the low level driver. It would
> >> be useful to abstract the machine specific state to a defined enum
> >> that would be part of the fpga mgr framework.  I remember we had
> >> something like that in the earliest version of fpga manager but it got
> >> changed to state rather than status for some reason.
> >>
> >
> > Adding a status function and a 'status' member to fpga manager sounds good.
> > But I'm not sure how to abstract these error codes, currently what we have
> > are all PR error codes for Intel FPGA device, e.g "PR FIFO overflow error",
> > "PR secure load error" and etc (see include/uapi/linux/intel-fpga.h). Is it
> > fine to let each driver to define how to use that 'status' for machine
> > specific status?
> 
> I looked at the list of errors in include/uapi/linux/intel-fpga.h.
> They all seem pretty generic to me except I am not clear what "secure
> load error" or "IP protocol error" mean and whether other
> architectures would have them.  But certainly things like crc error,
> incompatible bitstream, fifo overflow are generic.  So let's see if we
> can define all these in a way that's generic and just pass up a error
> number.  That way upper layers can know how to deal with them
> possibly.  I would take the word "PR" off these since the error set
> applies whether someone is doing full reconfig or partial reconfig.

Sure, good to me, we can make it this way and see.
Thanks for the suggestions. :)

Hao

> 
> >
> >> mgr->dev won't work for you for some of the things you are using fme->pdev for?
> >>
> >
> > Does 'fme->pdev' mean the platform device for fme? I think we use platform
> > device to represent the FME module, and FME module may have multiple sub
> > features, driver discovers these sub features via the 'Device Feature List'
> > in the PCIe Device Bar. PR is only one of the sub features under FME module,
> > if any FME module doesn't have PR sub feature, fpga-manager will not be
> > created at all. But this will not impact other sub features, e.g thermal
> > management, error reporting and etc, to create their own interfaces under
> > the platform device.
> 
> If we need it for private data, that's may be actually OK.   Just
> wondering whether the priv was needed.   I think that's the one
> element in the private data struct that was used privately instead of
> being used to pass info outside the framework.
> 
> >
> > Thanks
> > Hao
> >
> >> Alan
> >>
> >> > +
> >> > +       /* Find and get port device by index */
> >> > +       port = pdata->fpga_for_each_port(pdev, &fme->port_id,
> >> > +                                        fpga_port_check_id);
> >> > +       WARN_ON(!port);
> >> > +
> >> > +       /* Disable Port before PR */
> >> > +       fpga_port_disable(port);
> >> > +
> >> > +       ret = fpga_mgr_buf_load(mgr, &info, buf, port_pr.buffer_size);
> >> > +       port_pr.status = fme->pr_err;
> >> > +
> >> > +       /* Re-enable Port after PR finished */
> >> > +       fpga_port_enable(port);
> >> > +
> >> > +       put_device(&port->dev);
> >> > +
> >> > +unlock_exit:
> >> > +       mutex_unlock(&pdata->lock);
> >> > +       fpga_mgr_put(mgr);
> >> > +free_exit:
> >> > +       vfree(buf);
> >> > +       if (copy_to_user((void __user *)arg, &port_pr, minsz))
> >> > +               return -EFAULT;
> >> > +       return ret;
> >> > +}
> >> > +
> >> > +static int fpga_fme_pr_probe(struct platform_device *pdev)
> >> > +{
> >> > +       struct feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
> >> > +       struct fpga_fme *priv;
> >> > +       int ret;
> >> > +
> >> > +       mutex_lock(&pdata->lock);
> >> > +       priv = fpga_pdata_get_private(pdata);
> >> > +       ret = fpga_mgr_register(&pdata->dev->dev,
> >> > +               "Intel FPGA Manager", &fme_pr_ops, priv);
> >> > +       mutex_unlock(&pdata->lock);
> >> > +
> >> > +       return ret;
> >> > +}
> >> > +
> >> > +static int fpga_fme_pr_remove(struct platform_device *pdev)
> >> > +{
> >> > +       fpga_mgr_unregister(&pdev->dev);
> >> > +       return 0;
> >> > +}
> >> > +
> >> > +static int pr_mgmt_init(struct platform_device *pdev, struct feature *feature)
> >> > +{
> >> > +       int ret;
> >> > +
> >> > +       ret = fpga_fme_pr_probe(pdev);
> >> > +       if (ret)
> >> > +               return ret;
> >> > +
> >> > +       ret = sysfs_create_group(&pdev->dev.kobj, &pr_mgmt_attr_group);
> >> > +       if (ret)
> >> > +               fpga_fme_pr_remove(pdev);
> >> > +
> >> > +       return ret;
> >> > +}
> >> > +
> >> > +static void pr_mgmt_uinit(struct platform_device *pdev, struct feature *feature)
> >> > +{
> >> > +       sysfs_remove_group(&pdev->dev.kobj, &pr_mgmt_attr_group);
> >> > +       fpga_fme_pr_remove(pdev);
> >> > +}
> >> > +
> >> > +static long fme_pr_ioctl(struct platform_device *pdev, struct feature *feature,
> >> > +       unsigned int cmd, unsigned long arg)
> >> > +{
> >> > +       long ret;
> >> > +
> >> > +       switch (cmd) {
> >> > +       case FPGA_FME_PORT_PR:
> >> > +               ret = fme_pr(pdev, arg);
> >> > +               break;
> >> > +       default:
> >> > +               ret = -ENODEV;
> >> > +       }
> >> > +
> >> > +       return ret;
> >> > +}
> >> > +
> >> > +struct feature_ops pr_mgmt_ops = {
> >> > +       .init = pr_mgmt_init,
> >> > +       .uinit = pr_mgmt_uinit,
> >> > +       .ioctl = fme_pr_ioctl,
> >> > +};
> >> > diff --git a/drivers/fpga/intel/fme.h b/drivers/fpga/intel/fme.h
> >> > new file mode 100644
> >> > index 0000000..d6cb7ce
> >> > --- /dev/null
> >> > +++ b/drivers/fpga/intel/fme.h
> >> > @@ -0,0 +1,32 @@
> >> > +/*
> >> > + * Header file for Intel FPGA Management Engine (FME) Driver
> >> > + *
> >> > + * Copyright (C) 2017 Intel Corporation, Inc.
> >> > + *
> >> > + * Authors:
> >> > + *   Kang Luwei <luwei.kang@...el.com>
> >> > + *   Xiao Guangrong <guangrong.xiao@...ux.intel.com>
> >> > + *   Joseph Grecco <joe.grecco@...el.com>
> >> > + *   Enno Luebbers <enno.luebbers@...el.com>
> >> > + *   Tim Whisonant <tim.whisonant@...el.com>
> >> > + *   Ananda Ravuri <ananda.ravuri@...el.com>
> >> > + *   Henry Mitchel <henry.mitchel@...el.com>
> >> > + *
> >> > + * This work is licensed under a dual BSD/GPLv2 license. When using or
> >> > + * redistributing this file, you may do so under either license. See the
> >> > + * LICENSE.BSD file under this directory for the BSD license and see
> >> > + * the COPYING file in the top-level directory for the GPLv2 license.
> >> > + */
> >> > +
> >> > +#ifndef __INTEL_FME_H
> >> > +#define __INTEL_FME_H
> >> > +
> >> > +struct fpga_fme {
> >> > +       u8  port_id;
> >> > +       u64 pr_err;
> >> > +       struct feature_platform_data *pdata;
> >> > +};
> >> > +
> >> > +extern struct feature_ops pr_mgmt_ops;
> >> > +
> >> > +#endif
> >> > diff --git a/include/uapi/linux/intel-fpga.h b/include/uapi/linux/intel-fpga.h
> >> > index 992e556..77658316 100644
> >> > --- a/include/uapi/linux/intel-fpga.h
> >> > +++ b/include/uapi/linux/intel-fpga.h
> >> > @@ -18,6 +18,8 @@
> >> >  #ifndef _UAPI_LINUX_INTEL_FPGA_H
> >> >  #define _UAPI_LINUX_INTEL_FPGA_H
> >> >
> >> > +#include <linux/types.h>
> >> > +
> >> >  #define FPGA_API_VERSION 0
> >> >
> >> >  /*
> >> > @@ -30,6 +32,7 @@
> >> >  #define FPGA_MAGIC 0xB6
> >> >
> >> >  #define FPGA_BASE 0
> >> > +#define FME_BASE 0x80
> >> >
> >> >  /**
> >> >   * FPGA_GET_API_VERSION - _IO(FPGA_MAGIC, FPGA_BASE + 0)
> >> > @@ -49,4 +52,45 @@
> >> >
> >> >  #define FPGA_CHECK_EXTENSION   _IO(FPGA_MAGIC, FPGA_BASE + 1)
> >> >
> >> > +/* IOCTLs for FME file descriptor */
> >> > +
> >> > +/**
> >> > + * FPGA_FME_PORT_PR - _IOWR(FPGA_MAGIC, FME_BASE + 0, struct fpga_fme_port_pr)
> >> > + *
> >> > + * Driver does Partial Reconfiguration based on Port ID and Buffer (Image)
> >> > + * provided by caller.
> >> > + * Return: 0 on success, -errno on failure.
> >> > + * If FPGA_FME_PORT_PR returns -EIO, that indicates the HW has detected
> >> > + * some errors during PR, under this case, the user can fetch HW error code
> >> > + * from fpga_fme_port_pr.status. Each bit on the error code is used as the
> >> > + * index for the array created by DEFINE_FPGA_PR_ERR_MSG().
> >> > + * Otherwise, it is always zero.
> >> > + */
> >> > +
> >> > +#define DEFINE_FPGA_PR_ERR_MSG(_name_)                 \
> >> > +static const char * const _name_[] = {                 \
> >> > +       "PR operation error detected",                  \
> >> > +       "PR CRC error detected",                        \
> >> > +       "PR incompatiable bitstream error detected",    \
> >> > +       "PR IP protocol error detected",                \
> >> > +       "PR FIFO overflow error detected",              \
> >> > +       "Reserved",                                     \
> >> > +       "PR secure load error detected",                \
> >> > +}
> >> > +
> >> > +#define PR_MAX_ERR_NUM 7
> >> > +
> >> > +struct fpga_fme_port_pr {
> >> > +       /* Input */
> >> > +       __u32 argsz;            /* Structure length */
> >> > +       __u32 flags;            /* Zero for now */
> >> > +       __u32 port_id;
> >> > +       __u32 buffer_size;
> >> > +       __u64 buffer_address;   /* Userspace address to the buffer for PR */
> >> > +       /* Output */
> >> > +       __u64 status;           /* HW error code if ioctl returns -EIO */
> >> > +};
> >> > +
> >> > +#define FPGA_FME_PORT_PR       _IO(FPGA_MAGIC, FME_BASE + 0)
> >> > +
> >> >  #endif /* _UAPI_INTEL_FPGA_H */
> >> > --
> >> > 2.7.4
> >> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists