| 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
| ||
|
Message-ID: <CANk1AXQwN6-D9zo=G0YLK49P-1RyagfOc+Gs3PrsaqhvPkFnqQ@mail.gmail.com>
Date: Mon, 3 Apr 2017 11:30:55 -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 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).
> 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.
> 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.
>
>> 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
>> >
Powered by blists - more mailing lists