Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1581425 > unrolled thread

[RFC 7/8] fpga-region: add sysfs interface

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [RFC 7/8] fpga-region: add sysfs interface Alan Tull <atull@kernel.org> - 2017-02-15 17:20 +0100
    Re: [RFC 7/8] fpga-region: add sysfs interface Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2017-02-15 18:30 +0100
      Re: [RFC 7/8] fpga-region: add sysfs interface Alan Tull <delicious.quinoa@gmail.com> - 2017-02-15 18:50 +0100
        Re: [RFC 7/8] fpga-region: add sysfs interface Moritz Fischer <moritz.fischer@ettus.com> - 2017-02-15 19:00 +0100
        Re: [RFC 7/8] fpga-region: add sysfs interface Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2017-02-15 19:10 +0100
          Re: [RFC 7/8] fpga-region: add sysfs interface Alan Tull <delicious.quinoa@gmail.com> - 2017-02-15 19:30 +0100
            Re: [RFC 7/8] fpga-region: add sysfs interface Moritz Fischer <moritz.fischer@ettus.com> - 2017-02-15 19:40 +0100
            Re: [RFC 7/8] fpga-region: add sysfs interface Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2017-02-15 20:50 +0100
              Re: [RFC 7/8] fpga-region: add sysfs interface Alan Tull <delicious.quinoa@gmail.com> - 2017-02-16 00:00 +0100
                Re: [RFC 7/8] fpga-region: add sysfs interface Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2017-02-16 00:10 +0100
            Re: [RFC 7/8] fpga-region: add sysfs interface matthew.gerlach@linux.intel.com - 2017-02-15 21:10 +0100
              Re: [RFC 7/8] fpga-region: add sysfs interface Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2017-02-15 21:40 +0100
                Re: [RFC 7/8] fpga-region: add sysfs interface Moritz Fischer <moritz.fischer@ettus.com> - 2017-02-15 22:00 +0100
                  Re: [RFC 7/8] fpga-region: add sysfs interface Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2017-02-15 22:20 +0100
                    Re: [RFC 7/8] fpga-region: add sysfs interface Moritz Fischer <moritz.fischer@ettus.com> - 2017-02-15 22:40 +0100
                      Re: [RFC 7/8] fpga-region: add sysfs interface Alan Tull <delicious.quinoa@gmail.com> - 2017-02-15 23:50 +0100
                        Re: [RFC 7/8] fpga-region: add sysfs interface Moritz Fischer <moritz.fischer@ettus.com> - 2017-02-16 01:20 +0100
                          Re: [RFC 7/8] fpga-region: add sysfs interface Alan Tull <delicious.quinoa@gmail.com> - 2017-02-16 18:50 +0100
                            Re: [RFC 7/8] fpga-region: add sysfs interface Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2017-02-16 19:00 +0100
                              Re: [RFC 7/8] fpga-region: add sysfs interface Moritz Fischer <moritz.fischer@ettus.com> - 2017-02-16 19:20 +0100
                  Re: [RFC 7/8] fpga-region: add sysfs interface Yves Vandervennet <yves.vandervennet@linux.intel.com> - 2017-02-17 23:30 +0100
                    Re: [RFC 7/8] fpga-region: add sysfs interface Moritz Fischer <moritz.fischer@ettus.com> - 2017-02-18 03:40 +0100
                      RE: [RFC 7/8] fpga-region: add sysfs interface "Nadathur, Sundar" <sundar.nadathur@intel.com> - 2017-02-18 13:50 +0100
                        Re: [RFC 7/8] fpga-region: add sysfs interface Alan Tull <delicious.quinoa@gmail.com> - 2017-02-18 21:20 +0100
                          Re: [RFC 7/8] fpga-region: add sysfs interface Moritz Fischer <moritz.fischer@ettus.com> - 2017-02-18 22:00 +0100
                            Re: [RFC 7/8] fpga-region: add sysfs interface Alan Tull <delicious.quinoa@gmail.com> - 2017-02-19 16:10 +0100
                              Re: [RFC 7/8] fpga-region: add sysfs interface Alan Tull <delicious.quinoa@gmail.com> - 2017-02-20 00:20 +0100
                                Re: [RFC 7/8] fpga-region: add sysfs interface Moritz Fischer <moritz.fischer@ettus.com> - 2017-02-21 01:00 +0100
                                  Re: [RFC 7/8] fpga-region: add sysfs interface Alan Tull <delicious.quinoa@gmail.com> - 2017-02-21 19:40 +0100
                                    RE: [RFC 7/8] fpga-region: add sysfs interface "Nadathur, Sundar" <sundar.nadathur@intel.com> - 2017-02-22 04:20 +0100
                                      Re: [RFC 7/8] fpga-region: add sysfs interface Moritz Fischer <moritz.fischer@ettus.com> - 2017-02-22 04:50 +0100
                                        Re: [RFC 7/8] fpga-region: add sysfs interface Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2017-02-22 06:20 +0100
                                          Re: [RFC 7/8] fpga-region: add sysfs interface Moritz Fischer <moritz.fischer@ettus.com> - 2017-02-22 06:40 +0100
                                            RE: [RFC 7/8] fpga-region: add sysfs interface "Nadathur, Sundar" <sundar.nadathur@intel.com> - 2017-02-22 06:50 +0100
                                              Re: [RFC 7/8] fpga-region: add sysfs interface Moritz Fischer <moritz.fischer@ettus.com> - 2017-02-22 07:10 +0100
                                                Re: [RFC 7/8] fpga-region: add sysfs interface Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2017-02-22 17:50 +0100
                                                  Re: [RFC 7/8] fpga-region: add sysfs interface Moritz Fischer <moritz.fischer@ettus.com> - 2017-02-22 19:00 +0100
                                                    Re: [RFC 7/8] fpga-region: add sysfs interface Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2017-02-22 19:00 +0100
                                                      Re: [RFC 7/8] fpga-region: add sysfs interface Moritz Fischer <moritz.fischer@ettus.com> - 2017-02-22 19:00 +0100
                                            Re: [RFC 7/8] fpga-region: add sysfs interface Alan Tull <delicious.quinoa@gmail.com> - 2017-02-22 17:40 +0100
                                              Re: [RFC 7/8] fpga-region: add sysfs interface Moritz Fischer <moritz.fischer@ettus.com> - 2017-02-22 17:50 +0100
                                                Re: [RFC 7/8] fpga-region: add sysfs interface Alan Tull <delicious.quinoa@gmail.com> - 2017-02-22 18:00 +0100
                                    Re: [RFC 7/8] fpga-region: add sysfs interface Moritz Fischer <moritz.fischer@ettus.com> - 2017-02-28 00:00 +0100
                                      Re: [RFC 7/8] fpga-region: add sysfs interface matthew.gerlach@linux.intel.com - 2017-02-28 07:30 +0100
                                    Re: [RFC 7/8] fpga-region: add sysfs interface Alan Tull <delicious.quinoa@gmail.com> - 2017-02-28 13:40 +0100
          Re: [RFC 7/8] fpga-region: add sysfs interface Anatolij Gustschin <agust@denx.de> - 2017-02-15 22:30 +0100

Page 1 of 3  [1] 2 3  Next page →

#1581425 — [RFC 7/8] fpga-region: add sysfs interface

Add a sysfs interface to control programming FPGA.

Each fpga-region will get the following files which set values
in the fpga_image_info struct for that region.  More files will
need to be added as fpga_image_info expands.

firmware_name
* writing a name of a FPGA image file to firmware_name causes the
  FPGA region to write the FPGA

partial_config
* 0 : full reconfiguration
* 1 : partial reconfiguration

unfreeze_timeout
* Timeout for waiting for a freeze bridge to enable traffic

freeze_timeout
* Timeout for waiting for a freeze bridge to disable traffic

Signed-off-by: Alan Tull <atull@kernel.org>
---
 drivers/fpga/Kconfig       |   8 ++
 drivers/fpga/fpga-region.c | 241 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 249 insertions(+)

diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
index be9c23d..6455e02 100644
--- a/drivers/fpga/Kconfig
+++ b/drivers/fpga/Kconfig
@@ -21,6 +21,14 @@ config FPGA_REGION
 	  and the FPGA Bridges associated with either a reconfigurable
 	  region of an FPGA or a whole FPGA.
 
+config FPGA_REGION_SYSFS
+       bool "FPGA Region Sysfs"
+	depends on FPGA_REGION
+	help
+	  FPGA Region sysfs interface.  This creates sysfs file for each
+	  FPGA Region under /sys/class/fpga_region/ to show status and
+	  control programming FPGA regions.
+
 config OF_FPGA_REGION
 	tristate "FPGA Region Device Tree Overlay Support"
 	depends on FPGA_REGION
diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
index 5690237..a63bc6c 100644
--- a/drivers/fpga/fpga-region.c
+++ b/drivers/fpga/fpga-region.c
@@ -105,6 +105,243 @@ EXPORT_SYMBOL_GPL(fpga_region_ovl_image_info);
 
 #endif /* CONFIG_OF_FPGA_REGION */
 
+#if IS_ENABLED(CONFIG_FPGA_REGION_SYSFS)
+
+struct fpga_image_info *image_info_from_region(struct fpga_region *region)
+{
+	struct fpga_image_info *image_info;
+
+	/* If region has an overlay, display image_info from overlay. */
+	image_info = fpga_region_ovl_image_info(region);
+	if (!image_info)
+		image_info = region->image_info;
+
+	return image_info;
+}
+
+/*
+ * Controlling a region both by sysfs and by device tree overlays is
+ * not supported.
+ */
+static ssize_t firmware_name_show(struct device *dev,
+				  struct device_attribute *attr, char *buf)
+{
+	struct fpga_region *region = to_fpga_region(dev);
+	struct fpga_image_info *image_info;
+
+	image_info = image_info_from_region(region);
+
+	if (image_info && image_info->firmware_name)
+		return sprintf(buf, "%s\n", image_info->firmware_name);
+
+	return 0;
+}
+
+static ssize_t firmware_name_store(struct device *dev,
+				   struct device_attribute *attr,
+				   const char *buf, size_t count)
+{
+	struct fpga_region *region = to_fpga_region(dev);
+	char *firmware_name;
+	size_t len;
+	int ret;
+
+	/*
+	 * Controlling a region both by sysfs and by device tree overlays is
+	 * not supported.
+	 */
+	if (fpga_region_ovl_image_info(region))
+		return -EINVAL;
+
+	if (!region->image_info) {
+		region->image_info = fpga_region_alloc_image_info(region);
+		if (!region->image_info)
+			return -ENOMEM;
+	}
+
+	firmware_name = devm_kzalloc(dev, count, GFP_KERNEL);
+	if (!firmware_name)
+		return -ENOMEM;
+	pr_err("count = %d\n", count);
+	/* lose terminating \n */
+	strcpy(firmware_name, buf);
+	len = strlen(firmware_name);
+	if (firmware_name[len - 1] == '\n')
+		firmware_name[len - 1] = 0;
+	if (firmware_name[0] == 0) {
+		devm_kfree(dev, firmware_name);
+		firmware_name = NULL;
+	}
+
+	/* Release previous firmware name (if any). Save current one. */
+	if (region->image_info->firmware_name)
+		devm_kfree(dev, region->image_info->firmware_name);
+	region->image_info->firmware_name = firmware_name;
+
+	if (firmware_name) {
+		ret = fpga_region_program_fpga(region, region->image_info);
+		if (ret)
+			dev_err(dev,
+				"FPGA programming failed with value %d\n", ret);
+	} else {
+		/*
+		 * Writing null string to firmware_name will disable and put
+		 * the bridges (if there were any bridges in the bridge list).
+		 */
+		fpga_bridges_disable(&region->bridge_list);
+		if (region->get_bridges)
+			fpga_bridges_put(&region->bridge_list);
+		fpga_region_free_image_info(region, region->image_info);
+		region->image_info = NULL;
+	}
+
+	return count;
+}
+
+static ssize_t partial_config_show(struct device *dev,
+				   struct device_attribute *attr, char *buf)
+{
+	struct fpga_region *region = to_fpga_region(dev);
+	struct fpga_image_info *image_info;
+	int partial;
+
+	image_info = image_info_from_region(region);
+	if (!image_info)
+		return 0;
+
+	partial = !!(image_info->flags & FPGA_MGR_PARTIAL_RECONFIG);
+
+	return sprintf(buf, "%d\n", partial);
+}
+
+static ssize_t partial_config_store(struct device *dev,
+				    struct device_attribute *attr,
+				    const char *buf, size_t count)
+{
+	struct fpga_region *region = to_fpga_region(dev);
+	unsigned long val;
+	int ret;
+
+	if (fpga_region_ovl_image_info(region))
+		return -EINVAL;
+
+	if (!region->image_info) {
+		region->image_info = fpga_region_alloc_image_info(region);
+		if (!region->image_info)
+			return -ENOMEM;
+	}
+
+	ret = kstrtoul(buf, 0, &val);
+	if (ret)
+		return ret;
+
+	if (val == 1)
+		region->image_info->flags |= FPGA_MGR_PARTIAL_RECONFIG;
+	else if (val == 0)
+		region->image_info->flags &= ~FPGA_MGR_PARTIAL_RECONFIG;
+	else
+		return -EINVAL;
+
+	return count;
+}
+
+static ssize_t unfreeze_timeout_show(struct device *dev,
+				     struct device_attribute *attr,
+				     char *buf)
+{
+	struct fpga_region *region = to_fpga_region(dev);
+	struct fpga_image_info *image_info;
+
+	image_info = image_info_from_region(region);
+	if (!image_info)
+		return 0;
+
+	return sprintf(buf, "%d\n", image_info->enable_timeout_us);
+}
+
+static ssize_t unfreeze_timeout_store(struct device *dev,
+				      struct device_attribute *attr,
+				      const char *buf,
+				      size_t count)
+{
+	struct fpga_region *region = to_fpga_region(dev);
+	unsigned long val;
+	int ret;
+
+	if (fpga_region_ovl_image_info(region))
+		return -EINVAL;
+
+	if (!region->image_info) {
+		region->image_info = fpga_region_alloc_image_info(region);
+		if (!region->image_info)
+			return -ENOMEM;
+	}
+
+	ret = kstrtoul(buf, 0, &val);
+	if (ret)
+		return ret;
+
+	region->image_info->enable_timeout_us = val;
+
+	return count;
+}
+
+static ssize_t freeze_timeout_show(struct device *dev,
+				   struct device_attribute *attr, char *buf)
+{
+	struct fpga_region *region = to_fpga_region(dev);
+	struct fpga_image_info *image_info;
+
+	image_info = image_info_from_region(region);
+	if (!image_info)
+		return 0;
+
+	return sprintf(buf, "%d\n", image_info->disable_timeout_us);
+}
+
+static ssize_t freeze_timeout_store(struct device *dev,
+				    struct device_attribute *attr,
+				    const char *buf,
+				    size_t count)
+{
+	struct fpga_region *region = to_fpga_region(dev);
+	unsigned long val;
+	int ret;
+
+	if (fpga_region_ovl_image_info(region))
+		return -EINVAL;
+
+	if (!region->image_info) {
+		region->image_info = fpga_region_alloc_image_info(region);
+		if (!region->image_info)
+			return -ENOMEM;
+	}
+
+	ret = kstrtoul(buf, 0, &val);
+	if (ret)
+		return ret;
+
+	region->image_info->disable_timeout_us = val;
+
+	return count;
+}
+
+static DEVICE_ATTR_RW(firmware_name);
+static DEVICE_ATTR_RW(partial_config);
+static DEVICE_ATTR_RW(unfreeze_timeout);
+static DEVICE_ATTR_RW(freeze_timeout);
+
+static struct attribute *fpga_region_attrs[] = {
+	&dev_attr_firmware_name.attr,
+	&dev_attr_partial_config.attr,
+	&dev_attr_unfreeze_timeout.attr,
+	&dev_attr_freeze_timeout.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(fpga_region);
+
+#endif /* CONFIG_FPGA_REGION_SYSFS */
+
 /**
  * fpga_region_get - get an exclusive reference to a fpga region
  * @region: FPGA Region struct
@@ -288,6 +525,10 @@ static int __init fpga_region_init(void)
 	if (IS_ERR(fpga_region_class))
 		return PTR_ERR(fpga_region_class);
 
+#if IS_ENABLED(CONFIG_FPGA_REGION_SYSFS)
+	fpga_region_class->dev_groups = fpga_region_groups;
+#endif /* CONFIG_FPGA_REGION_SYSFS */
+
 	fpga_region_class->dev_release = fpga_region_dev_release;
 
 	return 0;
-- 
2.7.4

[toc] | [next] | [standalone]

#1581491

On Wed, Feb 15, 2017 at 10:14:20AM -0600, Alan Tull wrote:
> Add a sysfs interface to control programming FPGA.
> 
> Each fpga-region will get the following files which set values
> in the fpga_image_info struct for that region.  More files will
> need to be added as fpga_image_info expands.
> 
> firmware_name
> * writing a name of a FPGA image file to firmware_name causes the
>   FPGA region to write the FPGA
> 
> partial_config
> * 0 : full reconfiguration
> * 1 : partial reconfiguration

This is really a property of the bitfile. It would be really nice to
have a saner system for describing the bitfiles that doesn't rely on
so much out of band stuff.

Eg when doing partial reconfiguration it would be really sane to have
some checks that the full bitfile is the correct basis for the partial
bitfile.

It also seems link Zynq needs an encrypted/not encrypted flag..

I wonder if we should require a Linux specific header on the bitfile
instead? That would make the bitfile self describing at least.

Jason

[toc] | [prev] | [next] | [standalone]

#1581499

On Wed, Feb 15, 2017 at 11:21 AM, Jason Gunthorpe
<jgunthorpe@obsidianresearch.com> wrote:
> On Wed, Feb 15, 2017 at 10:14:20AM -0600, Alan Tull wrote:
>> Add a sysfs interface to control programming FPGA.
>>
>> Each fpga-region will get the following files which set values
>> in the fpga_image_info struct for that region.  More files will
>> need to be added as fpga_image_info expands.
>>
>> firmware_name
>> * writing a name of a FPGA image file to firmware_name causes the
>>   FPGA region to write the FPGA
>>
>> partial_config
>> * 0 : full reconfiguration
>> * 1 : partial reconfiguration
>
> This is really a property of the bitfile. It would be really nice to
> have a saner system for describing the bitfiles that doesn't rely on
> so much out of band stuff.
>
> Eg when doing partial reconfiguration it would be really sane to have
> some checks that the full bitfile is the correct basis for the partial
> bitfile.
>
> It also seems link Zynq needs an encrypted/not encrypted flag..
>
> I wonder if we should require a Linux specific header on the bitfile
> instead? That would make the bitfile self describing at least.

Hi Jason,

I agree.  I've heard some discussions about adding a header.  We would
want it to not be manufacturer or fpga device specific. That would be
nice and would eliminate some of this struct.  We would need a tool to
add the header, given a bitstream and some info about the bitstream.
If the tool communicated seamlessly with vendor's tools that would be
nice, but that is complicated to get that to happen.  So far nobody
has posted their proposals to the mailing list.

Alan

>
> Jason

[toc] | [prev] | [next] | [standalone]

#1581519

[Multipart message — attachments visible in raw view] — view raw

On Wed, Feb 15, 2017 at 11:46:01AM -0600, Alan Tull wrote:
> On Wed, Feb 15, 2017 at 11:21 AM, Jason Gunthorpe
> <jgunthorpe@obsidianresearch.com> wrote:
> > On Wed, Feb 15, 2017 at 10:14:20AM -0600, Alan Tull wrote:
> >> Add a sysfs interface to control programming FPGA.
> >>
> >> Each fpga-region will get the following files which set values
> >> in the fpga_image_info struct for that region.  More files will
> >> need to be added as fpga_image_info expands.
> >>
> >> firmware_name
> >> * writing a name of a FPGA image file to firmware_name causes the
> >>   FPGA region to write the FPGA
> >>
> >> partial_config
> >> * 0 : full reconfiguration
> >> * 1 : partial reconfiguration
> >
> > This is really a property of the bitfile. It would be really nice to
> > have a saner system for describing the bitfiles that doesn't rely on
> > so much out of band stuff.

Agreed.
> >
> > Eg when doing partial reconfiguration it would be really sane to have
> > some checks that the full bitfile is the correct basis for the partial
> > bitfile.
> >
> > It also seems link Zynq needs an encrypted/not encrypted flag..

Well, we could also run always at half rate and not benefit from faster
config for the non-encrypted case ;-)

> > I wonder if we should require a Linux specific header on the bitfile
> > instead? That would make the bitfile self describing at least.

> I agree.  I've heard some discussions about adding a header.  We would
> want it to not be manufacturer or fpga device specific. That would be
> nice and would eliminate some of this struct.  We would need a tool to
> add the header, given a bitstream and some info about the bitstream.
> If the tool communicated seamlessly with vendor's tools that would be
> nice, but that is complicated to get that to happen.  So far nobody
> has posted their proposals to the mailing list.

Well, there's not that many vendors out there. If we can figure out a
format and stick to it, keep it reasonably extensible, 'the vendors'
will eventually adopt it.

Cheers,

Moritz

[toc] | [prev] | [next] | [standalone]

#1581527

On Wed, Feb 15, 2017 at 11:46:01AM -0600, Alan Tull wrote:
> I agree.  I've heard some discussions about adding a header.  We would
> want it to not be manufacturer or fpga device specific. That would be
> nice and would eliminate some of this struct.  We would need a tool to
> add the header, given a bitstream and some info about the bitstream.
> If the tool communicated seamlessly with vendor's tools that would be
> nice, but that is complicated to get that to happen.  So far nobody
> has posted their proposals to the mailing list.

Okay, we've had success using a HTTP style plain text header for the
last 15 years. Here is a header example:

BIT/1.0
Bit-Order: reversed
Builder: jgg
Content-Length: 9987064
Date: Thu, 19 Jan 2017 06:22:42 GMT
Design: tluna
Device: 7k355t
GIT-TOT: 60da4e9e8e9610e490ddeb4e5880778938f80691
Package: ffg901
Pad: xxxx
Speed: 2
Speed-File: PRODUCTION 1.12 2014-09-11
Synplify-Ver: Pro I-2014.03-1 , Build 016R, Mar 24 2014
Xilinx-Ver: Vivado v.2016.1 (lin64) Build 1538259 Fri Apr  8 15:45:23 MDT 2016

[raw bitfile follows, start byte in the file is aligned for DMA]

The plaintext format allows a fair amount of flexibility, eg I could
include the linux header for partial/encrypt along with my usual
headers for identification.

So along those lines I'd suggest the basic Linux format to be

Linux_FPGA_BIT/1.0
FPGA-Device: xc7k355t-ffg901-2    # Allow the kernel driver to check, if it can
# Enable partial reconfiguration and require the full bitfile to have
# the ID 'xxx'
Partial-Reconfiguration-Basis-ID: xxxx
# This is a full bitfile with unique tag xxxx
FPGA-ID: xxxx 
Encrypted: yes/no   # Enable decryption if the driver needs to be told
Pad: xxxx           # Enough 'x' characters to align the bitfile

[raw bitfile follows, start byte in the file is aligned for DMA]

I can publish a version of my python script which produces these files
from typical Xilinx output..

The kernel could detect the bitfile starts with 'Linux_FPGA_BIT/1.0\n'
and then proceed to decode the header providing compat with the
current scheme.

This is usually the sort of stuff I'd punt to userspace, but since the
kernel is doing request_firmware it is hard to see how that is an
option in this case...

Jason

[toc] | [prev] | [next] | [standalone]

#1581556

On Wed, Feb 15, 2017 at 12:06 PM, Jason Gunthorpe
<jgunthorpe@obsidianresearch.com> wrote:

Hi Jason,

> On Wed, Feb 15, 2017 at 11:46:01AM -0600, Alan Tull wrote:
>> I agree.  I've heard some discussions about adding a header.  We would
>> want it to not be manufacturer or fpga device specific. That would be
>> nice and would eliminate some of this struct.  We would need a tool to
>> add the header, given a bitstream and some info about the bitstream.
>> If the tool communicated seamlessly with vendor's tools that would be
>> nice, but that is complicated to get that to happen.  So far nobody
>> has posted their proposals to the mailing list.
>
> Okay, we've had success using a HTTP style plain text header for the
> last 15 years. Here is a header example:
>
> BIT/1.0
> Bit-Order: reversed
> Builder: jgg
> Content-Length: 9987064
> Date: Thu, 19 Jan 2017 06:22:42 GMT
> Design: tluna
> Device: 7k355t
> GIT-TOT: 60da4e9e8e9610e490ddeb4e5880778938f80691
> Package: ffg901
> Pad: xxxx
> Speed: 2
> Speed-File: PRODUCTION 1.12 2014-09-11
> Synplify-Ver: Pro I-2014.03-1 , Build 016R, Mar 24 2014
> Xilinx-Ver: Vivado v.2016.1 (lin64) Build 1538259 Fri Apr  8 15:45:23 MDT 2016
>
> [raw bitfile follows, start byte in the file is aligned for DMA]
>
> The plaintext format allows a fair amount of flexibility, eg I could
> include the linux header for partial/encrypt along with my usual
> headers for identification.
>
> So along those lines I'd suggest the basic Linux format to be
>
> Linux_FPGA_BIT/1.0
> FPGA-Device: xc7k355t-ffg901-2    # Allow the kernel driver to check, if it can
> # Enable partial reconfiguration and require the full bitfile to have
> # the ID 'xxx'
> Partial-Reconfiguration-Basis-ID: xxxx
> # This is a full bitfile with unique tag xxxx
> FPGA-ID: xxxx
> Encrypted: yes/no   # Enable decryption if the driver needs to be told
> Pad: xxxx           # Enough 'x' characters to align the bitfile
>
> [raw bitfile follows, start byte in the file is aligned for DMA]
>
> I can publish a version of my python script which produces these files
> from typical Xilinx output..
>
> The kernel could detect the bitfile starts with 'Linux_FPGA_BIT/1.0\n'
> and then proceed to decode the header providing compat with the
> current scheme.
>
> This is usually the sort of stuff I'd punt to userspace, but since the
> kernel is doing request_firmware it is hard to see how that is an
> option in this case...

I like how extensible (and readable!) this is.  It wouldn't take much
kernel code to add this.  I'd like to see the python script.

Alan

>
> Jason

[toc] | [prev] | [next] | [standalone]

#1581560

Hi Jason, Alan

On Wed, Feb 15, 2017 at 7:23 PM, Alan Tull <delicious.quinoa@gmail.com> wrote:

>> This is usually the sort of stuff I'd punt to userspace, but since the
>> kernel is doing request_firmware it is hard to see how that is an
>> option in this case...
>
> I like how extensible (and readable!) this is.  It wouldn't take much
> kernel code to add this.  I'd like to see the python script.

We could also use something dts based like FIT in u-boot.
Just an idea. Downside is it would need a compiler (dtc)

Thanks,
Moritz

[toc] | [prev] | [next] | [standalone]

#1581602

On Wed, Feb 15, 2017 at 12:23:28PM -0600, Alan Tull wrote:
> > This is usually the sort of stuff I'd punt to userspace, but since the
> > kernel is doing request_firmware it is hard to see how that is an
> > option in this case...
> 
> I like how extensible (and readable!) this is.  It wouldn't take much
> kernel code to add this.  I'd like to see the python script.

Sure, attached

Jason

#!/usr/bin/env python
# COPYRIGHT (c) 2016 Obsidian Research Corporation.
# Permission is hereby granted, free of charge, to any person obtaining a copy
# of this software and associated documentation files (the "Software"), to deal
# in the Software without restriction, including without limitation the rights
# to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
# copies of the Software, and to permit persons to whom the Software is
# furnished to do so, subject to the following conditions:

# The above copyright notice and this permission notice shall be included in
# all copies or substantial portions of the Software.

# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
# IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
# FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
# AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
# LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
# OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
# THE SOFTWARE.

import subprocess,re,string,os,stat,mmap,sys,base64;
import argparse
import time;

def timeRFC1123():
    # Python doesn't have this as a built in
    return subprocess.check_output(["date","-u",'+%a, %02d %b %Y %T GMT']).strip();

def getTOT():
    """Return the top of tree commit hash from git"""
    try:
        HEAD = subprocess.check_output(["git","rev-parse","--verify","HEAD"]).strip();
    except subprocess.CalledProcessError:
        return "??? %s"%(os.getcwd());

    dirty = subprocess.check_output(["git","diff","--stat"]).strip();
    if dirty:
        return "%s-dirty"%(HEAD);
    return HEAD;

def parseISEPar(f,fmts):
    """Read Xilinx ISE par log files to get relevant design information"""
    f = string.join(f.readlines());
    g = re.search('"([^"]+)" is an NCD, version [0-9.]+, device (\S+), package (\S+), speed -(\d+)',f).groups();
    fmts.update({"Design": g[0],
                 "Device": g[1],
                 "Package": g[2],
                 "Speed": g[3]});
    fmts["PAR-Ver"] = re.search("(Release \S+ par \S+(?: \(\S+\))?)\n",f).groups()[0]
    fmts["Speed-File"] = re.search('Device speed data version:\s+"([^"]+)"',f).groups()[0];

def parseVivadoTwr(f,fmts):
    """Read Vivado 'report_timing' log files to get relevant design information"""
    f = string.join(f.readlines(1024));
    g = re.search('Design\s+:\s+(\S+)',f).groups();
    fmts["Design"] = g[0];

    g = re.search('Speed File\s+:\s+-(\d+)\s+(.+)',f);
    if g is not None:
        g = g.groups();
        fmts["Speed"] = g[0];
        fmts["Speed-File"] = g[1];
        g = re.search('Device\s+:\s+(\S+)-(\S+)',f).groups();
        fmts["Device"] = g[0];
        fmts["Package"] = g[1];
    else:
        g = re.search('Part\s+:\s+Device=(\S+)\s+Package=(\S+)\s+Speed=-(\d+)\s+\((.+)\)',f).groups();
        fmts.update({"Device": g[0],
                     "Package": g[1],
                     "Speed": g[2],
                     "Speed-File": g[3]});

    g = re.search('Version\s+:\s+(.+)',f).groups();
    fmts["Xilinx-Ver"] = g[0];

def parseSrr(f,fmts):
    """Read Synplify log files to get relevent design information"""
    l = f.readline().strip();
    fmts["Synplify-Ver"] = re.match("#Build: Synplify (.*)",l).groups()[0];

def find_start(bitm):
    """Locate the start of the actual bitsream in a Xilinx .bit file.  Xilinx
       tools drop an Impact header in front of the sync word. The FPGA ignores
       everything prior to the sync word."""
    for I in range(len(bitm)):
        if (bitm[I] == '\xaa' and bitm[I+1] == '\x99' and
            bitm[I+2] == '\x55' and bitm[I+3] == '\x66'):
            return I;
    return 0;

def align_bitstream(fmts,alignment=8):
    """Adjust the header content so that the bitstream starts aligned. This is
    so we can mmap this file with the header and still DMA from it."""
    while True:
        hdr = ("YYBIT/1.0\n" +
               "\n".join("%s: %s"%(k,v) for k,v in sorted(fmts.iteritems())) +
               "\n\n");
        if len(hdr) % alignment == 0:
            return hdr;
        fmts["Pad"] = "x"*(alignment - ((len(hdr) + 6) % alignment));

def makeHeader(out,args):
    fmts = {
        "Builder": os.getenv("USERNAME",os.getenv("USER","???")),
        "Date": timeRFC1123(),
        "GIT-TOT": getTOT(),
        "Bit-Order": args.order,
    };
    for fn in args.logs:
        with open(fn) as F:
            if fn.endswith(".par"):
                parseISEPar(F,fmts);
            if fn.endswith(".srr"):
                parseSrr(F,fmts);
            if fn.endswith(".twr"):
                parseVivadoTwr(F,fmts);
            if fn.endswith(".tsr"):
                parseVivadoTwr(F,fmts);

    with open(args.bit) as bitf:
        bitlen = os.fstat(bitf.fileno())[stat.ST_SIZE];

        bitm = mmap.mmap(bitf.fileno(),bitlen,access=mmap.ACCESS_COPY);
        start = 0;

        # This is the format for our bit bang schemes. The pin labeled D0 is
        # taken from bit 7.
        if args.order == "reversed":
            for i in range(0,bitlen):
                v = ord(bitm[i]);
                bitm[i] = chr(((v & (1<<0)) << 7) |
                              ((v & (1<<1)) << 5) |
                              ((v & (1<<2)) << 3) |
                              ((v & (1<<3)) << 1) |
                              ((v & (1<<4)) >> 1) |
                              ((v & (1<<5)) >> 3) |
                              ((v & (1<<6)) >> 5) |
                              ((v & (1<<7)) >> 7));

        # This is the format DMA to devcfg on the Zynq wants, impact header
        # stripped, sync word in little endian and aligned.
        if args.order == "byte-reversed":
            start = find_start(bitm);
            for i in range(start,bitlen//4*4,4):
                bitm[i],bitm[i+1],bitm[i+2],bitm[i+3] = bitm[i+3],bitm[i+2],bitm[i+1],bitm[i];

        if start != 0:
            fmts["Impact-Header"] = base64.b64encode(bitm[:start]);

        fmts["Content-Length"] = bitlen - start;
        out.write(align_bitstream(fmts));

        out.write(bitm[start:]);

parser = argparse.ArgumentParser(description="Format a Xilinx .bit file into a ybf with the necessary headers")
parser.add_argument("--ybf",required=True,
                    help="Output filename");
parser.add_argument("--bit",required=True,
                    help="Input bit filename");
parser.add_argument("--archive",
                    help="Optional directory to place a timestamped hardlink");
parser.add_argument("--deps",
                    help="File to write makefile dependencies list to");
parser.add_argument("--order",default="reversed",
                    help="Byte or bit order to use for the raw data");
parser.add_argument('logs',nargs="+",
                    help="Log files to pull meta data out of")
args = parser.parse_args();

with open(args.ybf,"wt") as F:
    makeHeader(F,args);

if args.archive:
    os.link(args.ybf,os.path.join(args.archive,"%s-%s"%(os.path.basename(args.ybf),int(time.time()))));

[toc] | [prev] | [next] | [standalone]

#1581769

On Wed, Feb 15, 2017 at 1:49 PM, Jason Gunthorpe
<jgunthorpe@obsidianresearch.com> wrote:
> On Wed, Feb 15, 2017 at 12:23:28PM -0600, Alan Tull wrote:
>> > This is usually the sort of stuff I'd punt to userspace, but since the
>> > kernel is doing request_firmware it is hard to see how that is an
>> > option in this case...
>>
>> I like how extensible (and readable!) this is.  It wouldn't take much
>> kernel code to add this.  I'd like to see the python script.
>
> Sure, attached
>
> Jason

Hi Jason,

Thanks for sharing this.

So this script takes the bitfile and its build logs as input, parses
the build logs for image information, does some manipulations on bit
order as needed, and adds the header.  So it's really doing (at least)
two things: adding header info and doing bitorder changes where needed
so that the kernel won't need to do it.

Alan

>
> #!/usr/bin/env python
> # COPYRIGHT (c) 2016 Obsidian Research Corporation.
> # Permission is hereby granted, free of charge, to any person obtaining a copy
> # of this software and associated documentation files (the "Software"), to deal
> # in the Software without restriction, including without limitation the rights
> # to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> # copies of the Software, and to permit persons to whom the Software is
> # furnished to do so, subject to the following conditions:
>
> # The above copyright notice and this permission notice shall be included in
> # all copies or substantial portions of the Software.
>
> # THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> # IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> # FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
> # AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> # LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> # OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> # THE SOFTWARE.
>
> import subprocess,re,string,os,stat,mmap,sys,base64;
> import argparse
> import time;
>
> def timeRFC1123():
>     # Python doesn't have this as a built in
>     return subprocess.check_output(["date","-u",'+%a, %02d %b %Y %T GMT']).strip();
>
> def getTOT():
>     """Return the top of tree commit hash from git"""
>     try:
>         HEAD = subprocess.check_output(["git","rev-parse","--verify","HEAD"]).strip();
>     except subprocess.CalledProcessError:
>         return "??? %s"%(os.getcwd());
>
>     dirty = subprocess.check_output(["git","diff","--stat"]).strip();
>     if dirty:
>         return "%s-dirty"%(HEAD);
>     return HEAD;
>
> def parseISEPar(f,fmts):
>     """Read Xilinx ISE par log files to get relevant design information"""
>     f = string.join(f.readlines());
>     g = re.search('"([^"]+)" is an NCD, version [0-9.]+, device (\S+), package (\S+), speed -(\d+)',f).groups();
>     fmts.update({"Design": g[0],
>                  "Device": g[1],
>                  "Package": g[2],
>                  "Speed": g[3]});
>     fmts["PAR-Ver"] = re.search("(Release \S+ par \S+(?: \(\S+\))?)\n",f).groups()[0]
>     fmts["Speed-File"] = re.search('Device speed data version:\s+"([^"]+)"',f).groups()[0];
>
> def parseVivadoTwr(f,fmts):
>     """Read Vivado 'report_timing' log files to get relevant design information"""
>     f = string.join(f.readlines(1024));
>     g = re.search('Design\s+:\s+(\S+)',f).groups();
>     fmts["Design"] = g[0];
>
>     g = re.search('Speed File\s+:\s+-(\d+)\s+(.+)',f);
>     if g is not None:
>         g = g.groups();
>         fmts["Speed"] = g[0];
>         fmts["Speed-File"] = g[1];
>         g = re.search('Device\s+:\s+(\S+)-(\S+)',f).groups();
>         fmts["Device"] = g[0];
>         fmts["Package"] = g[1];
>     else:
>         g = re.search('Part\s+:\s+Device=(\S+)\s+Package=(\S+)\s+Speed=-(\d+)\s+\((.+)\)',f).groups();
>         fmts.update({"Device": g[0],
>                      "Package": g[1],
>                      "Speed": g[2],
>                      "Speed-File": g[3]});
>
>     g = re.search('Version\s+:\s+(.+)',f).groups();
>     fmts["Xilinx-Ver"] = g[0];
>
> def parseSrr(f,fmts):
>     """Read Synplify log files to get relevent design information"""
>     l = f.readline().strip();
>     fmts["Synplify-Ver"] = re.match("#Build: Synplify (.*)",l).groups()[0];
>
> def find_start(bitm):
>     """Locate the start of the actual bitsream in a Xilinx .bit file.  Xilinx
>        tools drop an Impact header in front of the sync word. The FPGA ignores
>        everything prior to the sync word."""
>     for I in range(len(bitm)):
>         if (bitm[I] == '\xaa' and bitm[I+1] == '\x99' and
>             bitm[I+2] == '\x55' and bitm[I+3] == '\x66'):
>             return I;
>     return 0;
>
> def align_bitstream(fmts,alignment=8):
>     """Adjust the header content so that the bitstream starts aligned. This is
>     so we can mmap this file with the header and still DMA from it."""
>     while True:
>         hdr = ("YYBIT/1.0\n" +
>                "\n".join("%s: %s"%(k,v) for k,v in sorted(fmts.iteritems())) +
>                "\n\n");
>         if len(hdr) % alignment == 0:
>             return hdr;
>         fmts["Pad"] = "x"*(alignment - ((len(hdr) + 6) % alignment));
>
> def makeHeader(out,args):
>     fmts = {
>         "Builder": os.getenv("USERNAME",os.getenv("USER","???")),
>         "Date": timeRFC1123(),
>         "GIT-TOT": getTOT(),
>         "Bit-Order": args.order,
>     };
>     for fn in args.logs:
>         with open(fn) as F:
>             if fn.endswith(".par"):
>                 parseISEPar(F,fmts);
>             if fn.endswith(".srr"):
>                 parseSrr(F,fmts);
>             if fn.endswith(".twr"):
>                 parseVivadoTwr(F,fmts);
>             if fn.endswith(".tsr"):
>                 parseVivadoTwr(F,fmts);
>
>     with open(args.bit) as bitf:
>         bitlen = os.fstat(bitf.fileno())[stat.ST_SIZE];
>
>         bitm = mmap.mmap(bitf.fileno(),bitlen,access=mmap.ACCESS_COPY);
>         start = 0;
>
>         # This is the format for our bit bang schemes. The pin labeled D0 is
>         # taken from bit 7.
>         if args.order == "reversed":
>             for i in range(0,bitlen):
>                 v = ord(bitm[i]);
>                 bitm[i] = chr(((v & (1<<0)) << 7) |
>                               ((v & (1<<1)) << 5) |
>                               ((v & (1<<2)) << 3) |
>                               ((v & (1<<3)) << 1) |
>                               ((v & (1<<4)) >> 1) |
>                               ((v & (1<<5)) >> 3) |
>                               ((v & (1<<6)) >> 5) |
>                               ((v & (1<<7)) >> 7));
>
>         # This is the format DMA to devcfg on the Zynq wants, impact header
>         # stripped, sync word in little endian and aligned.
>         if args.order == "byte-reversed":
>             start = find_start(bitm);
>             for i in range(start,bitlen//4*4,4):
>                 bitm[i],bitm[i+1],bitm[i+2],bitm[i+3] = bitm[i+3],bitm[i+2],bitm[i+1],bitm[i];
>
>         if start != 0:
>             fmts["Impact-Header"] = base64.b64encode(bitm[:start]);
>
>         fmts["Content-Length"] = bitlen - start;
>         out.write(align_bitstream(fmts));
>
>         out.write(bitm[start:]);
>
> parser = argparse.ArgumentParser(description="Format a Xilinx .bit file into a ybf with the necessary headers")
> parser.add_argument("--ybf",required=True,
>                     help="Output filename");
> parser.add_argument("--bit",required=True,
>                     help="Input bit filename");
> parser.add_argument("--archive",
>                     help="Optional directory to place a timestamped hardlink");
> parser.add_argument("--deps",
>                     help="File to write makefile dependencies list to");
> parser.add_argument("--order",default="reversed",
>                     help="Byte or bit order to use for the raw data");
> parser.add_argument('logs',nargs="+",
>                     help="Log files to pull meta data out of")
> args = parser.parse_args();
>
> with open(args.ybf,"wt") as F:
>     makeHeader(F,args);
>
> if args.archive:
>     os.link(args.ybf,os.path.join(args.archive,"%s-%s"%(os.path.basename(args.ybf),int(time.time()))));

[toc] | [prev] | [next] | [standalone]

#1581784

On Wed, Feb 15, 2017 at 04:49:58PM -0600, Alan Tull wrote:

> So this script takes the bitfile and its build logs as input, parses
> the build logs for image information, does some manipulations on bit
> order as needed, and adds the header.  So it's really doing (at least)
> two things: adding header info and doing bitorder changes where needed
> so that the kernel won't need to do it.

Yes. This mangling is basically mandatory for Zynq due to how DevCfg
works, what Xilinx tools emit, and the desire to avoid copying the
bitfile.

Other cases are less essential, eg a gpio driver could do the
bit-reversal internally. We did the swap when writing the image
because the speed up was very noticable when the programming hardware
was a < 100MHz CPU.

It would be trivial to add Altera support, it really just needs a
similar build log parser for Altera's format to extract similar
information.

Jason

[toc] | [prev] | [next] | [standalone]

#1581628

Hi Jason, Alan, and Moritz,

On Wed, 15 Feb 2017, Alan Tull wrote:

> On Wed, Feb 15, 2017 at 12:06 PM, Jason Gunthorpe
> <jgunthorpe@obsidianresearch.com> wrote:
>
> Hi Jason,
>
>> On Wed, Feb 15, 2017 at 11:46:01AM -0600, Alan Tull wrote:
>>> I agree.  I've heard some discussions about adding a header.  We would
>>> want it to not be manufacturer or fpga device specific. That would be
>>> nice and would eliminate some of this struct.  We would need a tool to
>>> add the header, given a bitstream and some info about the bitstream.
>>> If the tool communicated seamlessly with vendor's tools that would be
>>> nice, but that is complicated to get that to happen.  So far nobody
>>> has posted their proposals to the mailing list.

It seems pretty clear that there is a set of meta data associated with 
a fpga bitstream to allow that bit stream to be authenticated, decrypted, 
and configured into the fpga.  When using device tree based fpga 
configuration, the meta data has been put into a device tree or device
tree overlay that is separate from the bitstream itself.

It does seem reasonable to consider combining the meta data with actual 
bitstream data.  The benefit of combining the meta data with the bitstream 
is that it simplifies the userspace/kernel interface to a single file 
transfer instead of having a growing number of sysfs entries for the meta 
data.

>>
>> Okay, we've had success using a HTTP style plain text header for the
>> last 15 years. Here is a header example:
>>
>> BIT/1.0
>> Bit-Order: reversed
>> Builder: jgg
>> Content-Length: 9987064
>> Date: Thu, 19 Jan 2017 06:22:42 GMT
>> Design: tluna
>> Device: 7k355t
>> GIT-TOT: 60da4e9e8e9610e490ddeb4e5880778938f80691
>> Package: ffg901
>> Pad: xxxx
>> Speed: 2
>> Speed-File: PRODUCTION 1.12 2014-09-11
>> Synplify-Ver: Pro I-2014.03-1 , Build 016R, Mar 24 2014
>> Xilinx-Ver: Vivado v.2016.1 (lin64) Build 1538259 Fri Apr  8 15:45:23 MDT 2016
>>
>> [raw bitfile follows, start byte in the file is aligned for DMA]
>>
>> The plaintext format allows a fair amount of flexibility, eg I could
>> include the linux header for partial/encrypt along with my usual
>> headers for identification.
>>
>> So along those lines I'd suggest the basic Linux format to be
>>
>> Linux_FPGA_BIT/1.0
>> FPGA-Device: xc7k355t-ffg901-2    # Allow the kernel driver to check, if it can
>> # Enable partial reconfiguration and require the full bitfile to have
>> # the ID 'xxx'
>> Partial-Reconfiguration-Basis-ID: xxxx
>> # This is a full bitfile with unique tag xxxx
>> FPGA-ID: xxxx
>> Encrypted: yes/no   # Enable decryption if the driver needs to be told
>> Pad: xxxx           # Enough 'x' characters to align the bitfile


The format of the meta data associated with a fpga bitstream is certainly 
a subject on its own.  HTTP style plain text is definately easy to 
understand and more importantly it is extendable.  On the other hand, it 
seems dangerous to be doing a lot of string parsing in the kernel.  Is 
there already an example of kernel code parsing an extendable data format? 
Depending on how the kernel is configured, the kernel code can parse a 
device tree blob.  I also think someone mentioned the FIT format which is 
closely related to device tree format.

Matthew Gerlach

>>
>> [raw bitfile follows, start byte in the file is aligned for DMA]
>>
>> I can publish a version of my python script which produces these files
>> from typical Xilinx output..
>>
>> The kernel could detect the bitfile starts with 'Linux_FPGA_BIT/1.0\n'
>> and then proceed to decode the header providing compat with the
>> current scheme.
>>
>> This is usually the sort of stuff I'd punt to userspace, but since the
>> kernel is doing request_firmware it is hard to see how that is an
>> option in this case...
>
> I like how extensible (and readable!) this is.  It wouldn't take much
> kernel code to add this.  I'd like to see the python script.
>
> Alan
>
>>
>> Jason
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

[toc] | [prev] | [next] | [standalone]

#1581645

On Wed, Feb 15, 2017 at 12:07:15PM -0800, matthew.gerlach@linux.intel.com wrote:

> The format of the meta data associated with a fpga bitstream is certainly a
> subject on its own.  HTTP style plain text is definately easy to understand
> and more importantly it is extendable.  On the other hand, it seems
> dangerous to be doing a lot of string parsing in the kernel.

It is fairly close to binary parsing.. The process is

- Find the first occurance of \n\n, must be less than XX bytes
- Memcpy that from the sg list into a linear buffer
- Replace all \n with \0

To access a key:
- Case insensitive search for START + "Key: " or \0 + "Key: "
- Return as a string the part after the match

This isn't the sort of string parsing that typically gets you into
trouble. If we can't code the above correctly then we will screw up
safe binary parsing of strings too :)

Jason

[toc] | [prev] | [next] | [standalone]

#1581653

Hi Jason,

On Wed, Feb 15, 2017 at 12:37 PM, Jason Gunthorpe
<jgunthorpe@obsidianresearch.com> wrote:
> On Wed, Feb 15, 2017 at 12:07:15PM -0800, matthew.gerlach@linux.intel.com wrote:
>
>> The format of the meta data associated with a fpga bitstream is certainly a
>> subject on its own.  HTTP style plain text is definately easy to understand
>> and more importantly it is extendable.  On the other hand, it seems
>> dangerous to be doing a lot of string parsing in the kernel.
>
> It is fairly close to binary parsing.. The process is
>
> - Find the first occurance of \n\n, must be less than XX bytes
> - Memcpy that from the sg list into a linear buffer
> - Replace all \n with \0
>
> To access a key:
> - Case insensitive search for START + "Key: " or \0 + "Key: "
> - Return as a string the part after the match
>
> This isn't the sort of string parsing that typically gets you into
> trouble. If we can't code the above correctly then we will screw up
> safe binary parsing of strings too :)

Well I don't know ;-) With something fdt based we already have parsers there,
compilers are already in tree. I'll take another look at the u-boot
code, I think their
FIT (Flattened Image Tree) would be a fairly good match for what we're
trying to do.

Cheers,
Moritz

[toc] | [prev] | [next] | [standalone]

#1581661

On Wed, Feb 15, 2017 at 12:54:27PM -0800, Moritz Fischer wrote:

> Well I don't know ;-) With something fdt based we already have
> parsers there,

Not sure.. How does incbin work in DTB?

We have the FPGA in a s/g list so we cannot pass the entire file to
libfdt - is that consistent with incbin?

Can we force a specific alignment for the included data?

How complex will the userspace tool be to make the image?

Jason

[toc] | [prev] | [next] | [standalone]

#1581683

Jason,

On Wed, Feb 15, 2017 at 1:15 PM, Jason Gunthorpe
<jgunthorpe@obsidianresearch.com> wrote:
> On Wed, Feb 15, 2017 at 12:54:27PM -0800, Moritz Fischer wrote:
>
>> Well I don't know ;-) With something fdt based we already have
>> parsers there,
>
> Not sure.. How does incbin work in DTB?
>
> We have the FPGA in a s/g list so we cannot pass the entire file to
> libfdt - is that consistent with incbin?

Well you could attach the (for lack of better word) blob to the beginning,
instead of doing incbin

> Can we force a specific alignment for the included data?

I'd say probably, but haven't checked.

> How complex will the userspace tool be to make the image?

Userspace can be as complex as it needs to be, imho, if it makes
kernel space easier & safer.

I'll need to do some more reading over the weekend before I can make
more sensible comments :)

Thanks,

Moritz

[toc] | [prev] | [next] | [standalone]

#1581727

On Wed, Feb 15, 2017 at 3:36 PM, Moritz Fischer
<moritz.fischer@ettus.com> wrote:
> Jason,
>
> On Wed, Feb 15, 2017 at 1:15 PM, Jason Gunthorpe
> <jgunthorpe@obsidianresearch.com> wrote:
>> On Wed, Feb 15, 2017 at 12:54:27PM -0800, Moritz Fischer wrote:
>>
>>> Well I don't know ;-) With something fdt based we already have
>>> parsers there,
>>
>> Not sure.. How does incbin work in DTB?
>>
>> We have the FPGA in a s/g list so we cannot pass the entire file to
>> libfdt - is that consistent with incbin?
>
> Well you could attach the (for lack of better word) blob to the beginning,
> instead of doing incbin
>
>> Can we force a specific alignment for the included data?
>
> I'd say probably, but haven't checked.
>
>> How complex will the userspace tool be to make the image?
>
> Userspace can be as complex as it needs to be, imho, if it makes
> kernel space easier & safer.
>
> I'll need to do some more reading over the weekend before I can make
> more sensible comments :)
>
> Thanks,
>
> Moritz

Another thought I have about this is that adding the header to
bitstreams can be a piece of independence from DT for systems that
aren't already using DT.  This includes x86 in Linux.  It also
includes other OS's that aren't using DT, they can reuse the same
image files without having to add dtc.  As much as I like DT, it is
something I'm having to think about.

Alan

[toc] | [prev] | [next] | [standalone]

#1582105

On Wed, Feb 15, 2017 at 2:42 PM, Alan Tull <delicious.quinoa@gmail.com> wrote:
> On Wed, Feb 15, 2017 at 3:36 PM, Moritz Fischer
> <moritz.fischer@ettus.com> wrote:
>> Jason,
>>
>> On Wed, Feb 15, 2017 at 1:15 PM, Jason Gunthorpe
>> <jgunthorpe@obsidianresearch.com> wrote:
>>> On Wed, Feb 15, 2017 at 12:54:27PM -0800, Moritz Fischer wrote:
>>>
>>>> Well I don't know ;-) With something fdt based we already have
>>>> parsers there,
>>>
>>> Not sure.. How does incbin work in DTB?
>>>
>>> We have the FPGA in a s/g list so we cannot pass the entire file to
>>> libfdt - is that consistent with incbin?
>>
>> Well you could attach the (for lack of better word) blob to the beginning,
>> instead of doing incbin
>>
>>> Can we force a specific alignment for the included data?
>>
>> I'd say probably, but haven't checked.
>>
>>> How complex will the userspace tool be to make the image?
>>
>> Userspace can be as complex as it needs to be, imho, if it makes
>> kernel space easier & safer.
>>
>> I'll need to do some more reading over the weekend before I can make
>> more sensible comments :)
>>
>> Thanks,
>>
>> Moritz
>
> Another thought I have about this is that adding the header to
> bitstreams can be a piece of independence from DT for systems that
> aren't already using DT.  This includes x86 in Linux.  It also
> includes other OS's that aren't using DT, they can reuse the same
> image files without having to add dtc.  As much as I like DT, it is
> something I'm having to think about.

Just to clarify:
I was proposing using the binary format of dts, not actually requiring
devicetree
for it to work. There's plenty of people running u-boot on x86 using FIT images
to boot.

W.r.t to Jason's script, it's there. Almost any company dealing with
Xilinx FPGAs
will have one of those. We have one, too. I recall having seen another one made
and shared by Mike @ topic.

While it's a good starting point ,I *really* don't like the idea
parsing user-land
provided strings in kernel space in a parser that we open-code.

Good discussion ;-)

Cheers,
Moritz

[toc] | [prev] | [next] | [standalone]

#1582733

On Wed, Feb 15, 2017 at 6:16 PM, Moritz Fischer
<moritz.fischer@ettus.com> wrote:
> On Wed, Feb 15, 2017 at 2:42 PM, Alan Tull <delicious.quinoa@gmail.com> wrote:
>> On Wed, Feb 15, 2017 at 3:36 PM, Moritz Fischer
>> <moritz.fischer@ettus.com> wrote:
>>> Jason,
>>>
>>> On Wed, Feb 15, 2017 at 1:15 PM, Jason Gunthorpe
>>> <jgunthorpe@obsidianresearch.com> wrote:
>>>> On Wed, Feb 15, 2017 at 12:54:27PM -0800, Moritz Fischer wrote:
>>>>
>>>>> Well I don't know ;-) With something fdt based we already have
>>>>> parsers there,
>>>>
>>>> Not sure.. How does incbin work in DTB?
>>>>
>>>> We have the FPGA in a s/g list so we cannot pass the entire file to
>>>> libfdt - is that consistent with incbin?
>>>
>>> Well you could attach the (for lack of better word) blob to the beginning,
>>> instead of doing incbin
>>>
>>>> Can we force a specific alignment for the included data?
>>>
>>> I'd say probably, but haven't checked.
>>>
>>>> How complex will the userspace tool be to make the image?
>>>
>>> Userspace can be as complex as it needs to be, imho, if it makes
>>> kernel space easier & safer.
>>>
>>> I'll need to do some more reading over the weekend before I can make
>>> more sensible comments :)
>>>
>>> Thanks,
>>>
>>> Moritz
>>
>> Another thought I have about this is that adding the header to
>> bitstreams can be a piece of independence from DT for systems that
>> aren't already using DT.  This includes x86 in Linux.  It also
>> includes other OS's that aren't using DT, they can reuse the same
>> image files without having to add dtc.  As much as I like DT, it is
>> something I'm having to think about.
>
> Just to clarify:
> I was proposing using the binary format of dts, not actually requiring
> devicetree
> for it to work. There's plenty of people running u-boot on x86 using FIT images
> to boot.

The FPGA images should not be required to have OS specific parts.
Some ahem non-Linux OS's that use FPGAs don't use device tree, so that
adds an extra complication for them unnecessarily.

>
> W.r.t to Jason's script, it's there. Almost any company dealing with
> Xilinx FPGAs
> will have one of those. We have one, too. I recall having seen another one made
> and shared by Mike @ topic.
>
> While it's a good starting point ,I *really* don't like the idea
> parsing user-land
> provided strings in kernel space in a parser that we open-code.

Why do you not like about it?  Jason posted some very clear practices
on how to do that properly and safely.

>
> Good discussion ;-)

Yes, I like it. :)

Alan

>
> Cheers,
> Moritz

[toc] | [prev] | [next] | [standalone]

#1582756

On Thu, Feb 16, 2017 at 11:47:08AM -0600, Alan Tull wrote:

> > Just to clarify: I was proposing using the binary format of dts,
> > not actually requiring devicetree for it to work. There's plenty
> > of people running u-boot on x86 using FIT images to boot.
> 
> The FPGA images should not be required to have OS specific parts.
> Some ahem non-Linux OS's that use FPGAs don't use device tree, so that
> adds an extra complication for them unnecessarily.

Not just that, but we parse the bitfile headers in user space as well.

Requiring people to use libfdt pretty much kills the idea because of
its GPL license.

As I've shown the plain text headers can be produced in a scripting
langauge, and are trivially consumed without much trouble. IHMO this
makes it more likely there would be adoption..

Jason

[toc] | [prev] | [next] | [standalone]

#1582814

On Thu, Feb 16, 2017 at 9:56 AM, Jason Gunthorpe
<jgunthorpe@obsidianresearch.com> wrote:
> On Thu, Feb 16, 2017 at 11:47:08AM -0600, Alan Tull wrote:
>
>> > Just to clarify: I was proposing using the binary format of dts,
>> > not actually requiring devicetree for it to work. There's plenty
>> > of people running u-boot on x86 using FIT images to boot.
>>
>> The FPGA images should not be required to have OS specific parts.
>> Some ahem non-Linux OS's that use FPGAs don't use device tree, so that
>> adds an extra complication for them unnecessarily.

That's a good point, I had assumed that pulling in a C library shouldn't
be an issue for any OS (u-boot can do it, BSD can do it, Linux can do it).
I have to admit I didn't think about Windows :)

> Not just that, but we parse the bitfile headers in user space as well.
>
> Requiring people to use libfdt pretty much kills the idea because of
> its GPL license.

<snip>

libfdt, however, is GPL/BSD dual-licensed.  That is, it may be used
either under the terms of the GPL, or under the terms of the 2-clause
BSD license (aka the ISC license).  The full terms of that license are
given in the copyright banners of each of the libfdt source files.
This is, in practice, equivalent to being BSD licensed, since the
terms of the BSD license are strictly more permissive than the GPL.

</snip>

> As I've shown the plain text headers can be produced in a scripting
> langauge, and are trivially consumed without much trouble. IHMO this
> makes it more likely there would be adoption..

If you provide a reasonably well documented format and make your tools
easy to use and integrate, I don't think that would be an issue.

I was really mainly concerned about the parsing userspace provided
strings in kernel being a security issue.

If everyone else feels http style plain text is the best we can do, so be it.

I wanted to look around and see how far I get with fdt, but anyway I'm quite
busy with other work at the moment. If someone comes around and writes
code that we can review, maybe that's better than not having a solution.

I'll see how far I get, maybe it turns out my proposal is a bad idea anyways
once I start writing actual code :)

Moritz

[toc] | [prev] | [next] | [standalone]

Page 1 of 3  [1] 2 3  Next page →

Back to top | Article view | linux.kernel

csiph-web