| 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 linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 4 Apr 2017 17:37:16 -0500
From: Alan Tull <atull@...nel.org>
To: Wu Hao <hao.wu@...el.com>
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 Tue, Apr 4, 2017 at 1:05 AM, Wu Hao <hao.wu@...el.com> wrote:
> 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?
No, just code that sets that variable in the struct before calling the
fpga_region_program_fpga function.
>
> 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.
>
Adding priv would make the interface for fpga-mgr non-uniform. The point
of having a fpga-mgr framework is that there
is the potential of the upper layers working for different FPGA devices.
If the interface for each FPGA device were different, that would then
be broken.
>>
>> > 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
Powered by blists - more mailing lists