| Message ID | 1487175261-7051-8-git-send-email-atull@kernel.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
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 -- 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
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 -- 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
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
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 -- 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
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 -- 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
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 -- 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
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())))); -- 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
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 > -- 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
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 -- 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
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 -- 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
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 -- 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
Hi Jason, On Wed, 15 Feb 2017 11:06:12 -0700 Jason Gunthorpe jgunthorpe@obsidianresearch.com wrote: ... >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... Interesting. We have a requirement that the complete history of FPGA configurations (basic configuration and the sequence of following partial reconfigurations) should be readable from FPGA manager framework on request. Each bitfile is associated with meta-data and one should be able to read this meta-data back for complete reconfiguration chain. When the kernel decodes a header, it could optionally store the data and allow to read it back on request. Thanks, Anatolij -- 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
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 -- 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
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 -- 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
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())))); -- 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
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 -- 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
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 -- 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
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 -- 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
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 -- 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
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 -- 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
Moritz, whatever solution we decide to go with has to work with other OS'es. The last thing we want to do is to have wrappers that are Linux specific. Yves On Wed, 15 Feb 2017, Moritz Fischer wrote: >>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 >>-- >>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 >> -- 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
On Fri, Feb 17, 2017 at 04:28:37PM -0600, Yves Vandervennet wrote: > Moritz, > > whatever solution we decide to go with has to work with other OS'es. The > last thing we want to do is to have wrappers that are Linux specific. I do agree that we should make sure the format is reasonably well documented. In my earlier email I pointed out several projects successfully integrating libfdt. There's nothing Linux specific about libfdt. FreeBSD uses it, U-Boot, Qemu ... I know nothing about how windows kernel development works, but I assume however one goes about making FPGA programming work there, someone will most likely have to write a kernel mode driver to take the job of the fpga-mgr framework. I assume this will be written in C or C++ or whatever people use these days for kernel development so pulling in libfdt shouldn't be too hard if we were to try that. To be clear: I did not suggest fdt to make it hard for other OSs, or because this is my personal pet project. I think we're more likely to get it right by reusing an existing format, with parsers that other people already successfully use. It does not have to be fdt (I suggested that because that was around), but I do think we certainly can do better than HTTP-eque plaintext headers. Thanks, Moritz -- 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
On February 17, 2017 6:30 PM, Moritz Fischer wrote: <quote> On Fri, Feb 17, 2017 at 04:28:37PM -0600, Yves Vandervennet wrote: > Moritz, > > whatever solution we decide to go with has to work with other OS'es. > The last thing we want to do is to have wrappers that are Linux specific. I do agree that we should make sure the format is reasonably well documented. In my earlier email I pointed out several projects successfully integrating libfdt. There's nothing Linux specific about libfdt. FreeBSD uses it, U-Boot, Qemu ... I know nothing about how windows kernel development works, but I assume however one goes about making FPGA programming work there, someone will most likely have to write a kernel mode driver to take the job of the fpga-mgr framework. I assume this will be written in C or C++ or whatever people use these days for kernel development so pulling in libfdt shouldn't be too hard if we were to try that. To be clear: I did not suggest fdt to make it hard for other OSs, or because this is my personal pet project. I think we're more likely to get it right by reusing an existing format, with parsers that other people already successfully use. It does not have to be fdt (I suggested that because that was around), but I do think we certainly can do better than HTTP-eque plaintext headers. Thanks, Moritz -- 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 </endquote> Hi all, Interesting discussion. The discussion so far has brought out many concerns such as OS independence. There is an existing format, well-known to developers, with widespread support, and which is quite extensible: Type-Length-Value triples. To elaborate, a TLV-based format has many advantages: * It is highly extensible in many ways -- You can express structures and arrays using TLVs. Our needs right now may seem limited but requirements grow over time. -- The space of Type values can be decomposed into standard pre-defined values that are in upstreamed code, and possibly experimental or feature-specific values. -- Forward compatibility: We can write parsers that can skip unexpected type values, thus allowing old parsers to work with new additions. With some tweaks, old parsers can also reject unexpected values in some ranges while accepting them in other ranges. * It is OS-independent. * It can be easily parsed, in kernel or user space. * It can be validated, in terms of Type values, acceptable lengths, etc. It is not directly human-readable but that can be easily addressed with a tool that parses TLVs. Compared to some other proposals: * Compared to DTs, TLVs are OS-independent. * Compared to strings as key-value pairs, TLVs can express structures/arrays, can be validated, etc. So, I suggest we use TLVs to express metadata in image files. Thank you very much, Sundar Nadathur -- 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
On Sat, Feb 18, 2017 at 6:45 AM, Nadathur, Sundar <sundar.nadathur@intel.com> wrote: > Hi all, > Interesting discussion. The discussion so far has brought out many concerns such as OS independence. There is an existing format, well-known to developers, with widespread support, and which is quite extensible: Type-Length-Value triples. > > To elaborate, a TLV-based format has many advantages: > * It is highly extensible in many ways > -- You can express structures and arrays using TLVs. Our needs right now may seem limited but requirements grow over time. > -- The space of Type values can be decomposed into standard pre-defined values that are in upstreamed code, and possibly experimental or feature-specific values. > -- Forward compatibility: We can write parsers that can skip unexpected type values, thus allowing old parsers to work with new additions. With some tweaks, old parsers can also reject unexpected values in some ranges while accepting them in other ranges. > * It is OS-independent. > * It can be easily parsed, in kernel or user space. > * It can be validated, in terms of Type values, acceptable lengths, etc. > > It is not directly human-readable but that can be easily addressed with a tool that parses TLVs. > > Compared to some other proposals: > * Compared to DTs, TLVs are OS-independent. > * Compared to strings as key-value pairs, TLVs can express structures/arrays, can be validated, etc. > > So, I suggest we use TLVs to express metadata in image files. > > Thank you very much, > Sundar Nadathur Hi Sundar, IIUC, each field is position dependent. One of the strengths of key value pairs is that any key can be added in any order and ones that aren't applicable for a particular architecture or use case can be left out. If the header is position dependent, it becomes less flexible. Once a field is added, we are stuck with it forever unless we drop it in some incremented version of the header format. Alan -- 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
On Sat, Feb 18, 2017 at 02:10:43PM -0600, Alan Tull wrote: > On Sat, Feb 18, 2017 at 6:45 AM, Nadathur, Sundar > <sundar.nadathur@intel.com> wrote: > > > Hi all, > > Interesting discussion. The discussion so far has brought out many concerns such as OS independence. There is an existing format, well-known to developers, with widespread support, and which is quite extensible: Type-Length-Value triples. > > > > To elaborate, a TLV-based format has many advantages: > > * It is highly extensible in many ways > > -- You can express structures and arrays using TLVs. Our needs right now may seem limited but requirements grow over time. > > -- The space of Type values can be decomposed into standard pre-defined values that are in upstreamed code, and possibly experimental or feature-specific values. > > -- Forward compatibility: We can write parsers that can skip unexpected type values, thus allowing old parsers to work with new additions. With some tweaks, old parsers can also reject unexpected values in some ranges while accepting them in other ranges. > > * It is OS-independent. > > * It can be easily parsed, in kernel or user space. Are there other users of the format? I have to admit I didn't look very long, but couldn't find any libs / existing code at a first glance. > > * It can be validated, in terms of Type values, acceptable lengths, etc. > > > > It is not directly human-readable but that can be easily addressed with a tool that parses TLVs. > > > > Compared to some other proposals: > > * Compared to DTs, TLVs are OS-independent. That's just alternative facts here. Just because Linux uses fdt for devicetree blobs it is *not* OS dependent. There are several (see last email) non-Linux users of fdt / libfdt. Thanks, Moritz -- 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
On Sat, Feb 18, 2017 at 2:45 PM, Moritz Fischer <moritz.fischer@ettus.com> wrote: > On Sat, Feb 18, 2017 at 02:10:43PM -0600, Alan Tull wrote: >> On Sat, Feb 18, 2017 at 6:45 AM, Nadathur, Sundar >> <sundar.nadathur@intel.com> wrote: >> >> > Hi all, >> > Interesting discussion. The discussion so far has brought out many concerns such as OS independence. There is an existing format, well-known to developers, with widespread support, and which is quite extensible: Type-Length-Value triples. >> > >> > To elaborate, a TLV-based format has many advantages: >> > * It is highly extensible in many ways >> > -- You can express structures and arrays using TLVs. Our needs right now may seem limited but requirements grow over time. Device tree can express arrays. >> > -- The space of Type values can be decomposed into standard pre-defined values that are in upstreamed code, and possibly experimental or feature-specific values. >> > -- Forward compatibility: We can write parsers that can skip unexpected type values, thus allowing old parsers to work with new additions. With some tweaks, old parsers can also reject unexpected values in some ranges while accepting them in other ranges. >> > * It is OS-independent. >> > * It can be easily parsed, in kernel or user space. > > Are there other users of the format? I have to admit I didn't look very > long, but couldn't find any libs / existing code at a first glance. Is there a standard you are looking at? Have you seen any use of TLV's in the Linux kernel you could point to? > >> > * It can be validated, in terms of Type values, acceptable lengths, etc. >> > >> > It is not directly human-readable but that can be easily addressed with a tool that parses TLVs. >> > >> > Compared to some other proposals: >> > * Compared to DTs, TLVs are OS-independent. > > That's just alternative facts here. Just because Linux uses fdt for > devicetree blobs it is *not* OS dependent. There are several (see > last email) non-Linux users of fdt / libfdt. > > Thanks, > > Moritz It is worth repeating that libdtc is GPL/BSD with the intent of allowing proprietary code to use libdtc. So license shouldn't be a barrier. Using device tree in the header would give us a way of doing enumeration at least for Linux, not sure if that kind of info can be used in Windows in some way. Alan -- 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
On Sun, Feb 19, 2017 at 9:00 AM, Alan Tull <delicious.quinoa@gmail.com> wrote: > On Sat, Feb 18, 2017 at 2:45 PM, Moritz Fischer > <moritz.fischer@ettus.com> wrote: >> On Sat, Feb 18, 2017 at 02:10:43PM -0600, Alan Tull wrote: >>> On Sat, Feb 18, 2017 at 6:45 AM, Nadathur, Sundar >>> <sundar.nadathur@intel.com> wrote: >>> >>> > Hi all, >>> > Interesting discussion. The discussion so far has brought out many concerns such as OS independence. There is an existing format, well-known to developers, with widespread support, and which is quite extensible: Type-Length-Value triples. >>> > >>> > To elaborate, a TLV-based format has many advantages: >>> > * It is highly extensible in many ways >>> > -- You can express structures and arrays using TLVs. Our needs right now may seem limited but requirements grow over time. > > Device tree can express arrays. > >>> > -- The space of Type values can be decomposed into standard pre-defined values that are in upstreamed code, and possibly experimental or feature-specific values. >>> > -- Forward compatibility: We can write parsers that can skip unexpected type values, thus allowing old parsers to work with new additions. With some tweaks, old parsers can also reject unexpected values in some ranges while accepting them in other ranges. >>> > * It is OS-independent. >>> > * It can be easily parsed, in kernel or user space. >> >> Are there other users of the format? I have to admit I didn't look very >> long, but couldn't find any libs / existing code at a first glance. > > Is there a standard you are looking at? Have you seen any use of TLV's > in the Linux kernel you could point to? > >> >>> > * It can be validated, in terms of Type values, acceptable lengths, etc. >>> > >>> > It is not directly human-readable but that can be easily addressed with a tool that parses TLVs. >>> > >>> > Compared to some other proposals: >>> > * Compared to DTs, TLVs are OS-independent. >> >> That's just alternative facts here. Just because Linux uses fdt for >> devicetree blobs it is *not* OS dependent. There are several (see >> last email) non-Linux users of fdt / libfdt. >> >> Thanks, >> >> Moritz > > It is worth repeating that libdtc is GPL/BSD with the intent of > allowing proprietary code to use libdtc. So license shouldn't be a barrier. > > Using device tree in the header would give us a way of doing enumeration at > least for Linux, not sure if that kind of info can be used in Windows > in some way. Actually, enumeration is the only advantage I see with DT. Currently I like key/value pairs because they are easily implemented and expandable without being rigid in any way. If we use key/value pairs, we could pass in child device info in one of the keys. It could be either a device tree overlay or an ACPI overlay. Or could just be left out. So platforms that aren't already using DT wouldn't have to. Platforms that are have a smooth road to enumeration. Alan > > Alan -- 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
Hi Alan, On Sun, Feb 19, 2017 at 3:16 PM, Alan Tull <delicious.quinoa@gmail.com> wrote: > On Sun, Feb 19, 2017 at 9:00 AM, Alan Tull <delicious.quinoa@gmail.com> wrote: >> On Sat, Feb 18, 2017 at 2:45 PM, Moritz Fischer >> <moritz.fischer@ettus.com> wrote: >>> On Sat, Feb 18, 2017 at 02:10:43PM -0600, Alan Tull wrote: >>>> On Sat, Feb 18, 2017 at 6:45 AM, Nadathur, Sundar >>>> <sundar.nadathur@intel.com> wrote: >>>> >>>> > Hi all, >>>> > Interesting discussion. The discussion so far has brought out many concerns such as OS independence. There is an existing format, well-known to developers, with widespread support, and which is quite extensible: Type-Length-Value triples. >>>> > >>>> > To elaborate, a TLV-based format has many advantages: >>>> > * It is highly extensible in many ways >>>> > -- You can express structures and arrays using TLVs. Our needs right now may seem limited but requirements grow over time. >> >> Device tree can express arrays. >> >>>> > -- The space of Type values can be decomposed into standard pre-defined values that are in upstreamed code, and possibly experimental or feature-specific values. >>>> > -- Forward compatibility: We can write parsers that can skip unexpected type values, thus allowing old parsers to work with new additions. With some tweaks, old parsers can also reject unexpected values in some ranges while accepting them in other ranges. >>>> > * It is OS-independent. >>>> > * It can be easily parsed, in kernel or user space. >>> >>> Are there other users of the format? I have to admit I didn't look very >>> long, but couldn't find any libs / existing code at a first glance. >> >> Is there a standard you are looking at? Have you seen any use of TLV's >> in the Linux kernel you could point to? >> >>> >>>> > * It can be validated, in terms of Type values, acceptable lengths, etc. >>>> > >>>> > It is not directly human-readable but that can be easily addressed with a tool that parses TLVs. >>>> > >>>> > Compared to some other proposals: >>>> > * Compared to DTs, TLVs are OS-independent. >>> >>> That's just alternative facts here. Just because Linux uses fdt for >>> devicetree blobs it is *not* OS dependent. There are several (see >>> last email) non-Linux users of fdt / libfdt. >>> >>> Thanks, >>> >>> Moritz >> >> It is worth repeating that libdtc is GPL/BSD with the intent of >> allowing proprietary code to use libdtc. So license shouldn't be a barrier. >> >> Using device tree in the header would give us a way of doing enumeration at >> least for Linux, not sure if that kind of info can be used in Windows >> in some way. > > Actually, enumeration is the only advantage I see with DT. Which seems to some point a separate issue to passing in image specific info such as encrypted or not, compressed or not or build info metadata. So I think in general we can still separate this out into: - Image specific values - Reconfiguration specific values > Currently I like key/value pairs because they are easily implemented > and expandable without being rigid in any way. > > If we use key/value pairs, we could pass in child device info > in one of the keys. It could be either a device tree overlay or an > ACPI overlay. Or could just be left out. So platforms that > aren't already using DT wouldn't have to. Platforms that > are have a smooth road to enumeration. I'm not sure if you can bundle up enumeration info *with* the image since you might e.g. load the same image (i.e. same header) into different FPGAs and the required update to the kernel state, i.e. live tree or ACPI would depend entirely on which FPGA you loaded the image into w.r.t busses it's connected to etc. I do think this info cannot be image specific, but needs to be passed in via something external such as a dt overlay. Cheers, Moritz -- 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
On Mon, Feb 20, 2017 at 5:49 PM, Moritz Fischer <moritz.fischer@ettus.com> wrote: > Hi Alan, > > On Sun, Feb 19, 2017 at 3:16 PM, Alan Tull <delicious.quinoa@gmail.com> wrote: >> On Sun, Feb 19, 2017 at 9:00 AM, Alan Tull <delicious.quinoa@gmail.com> wrote: >>> On Sat, Feb 18, 2017 at 2:45 PM, Moritz Fischer >>> <moritz.fischer@ettus.com> wrote: >>>> On Sat, Feb 18, 2017 at 02:10:43PM -0600, Alan Tull wrote: >>>>> On Sat, Feb 18, 2017 at 6:45 AM, Nadathur, Sundar >>>>> <sundar.nadathur@intel.com> wrote: >>>>> >>>>> > Hi all, >>>>> > Interesting discussion. The discussion so far has brought out many concerns such as OS independence. There is an existing format, well-known to developers, with widespread support, and which is quite extensible: Type-Length-Value triples. >>>>> > >>>>> > To elaborate, a TLV-based format has many advantages: >>>>> > * It is highly extensible in many ways >>>>> > -- You can express structures and arrays using TLVs. Our needs right now may seem limited but requirements grow over time. >>> >>> Device tree can express arrays. >>> >>>>> > -- The space of Type values can be decomposed into standard pre-defined values that are in upstreamed code, and possibly experimental or feature-specific values. >>>>> > -- Forward compatibility: We can write parsers that can skip unexpected type values, thus allowing old parsers to work with new additions. With some tweaks, old parsers can also reject unexpected values in some ranges while accepting them in other ranges. >>>>> > * It is OS-independent. >>>>> > * It can be easily parsed, in kernel or user space. >>>> >>>> Are there other users of the format? I have to admit I didn't look very >>>> long, but couldn't find any libs / existing code at a first glance. >>> >>> Is there a standard you are looking at? Have you seen any use of TLV's >>> in the Linux kernel you could point to? >>> >>>> >>>>> > * It can be validated, in terms of Type values, acceptable lengths, etc. >>>>> > >>>>> > It is not directly human-readable but that can be easily addressed with a tool that parses TLVs. >>>>> > >>>>> > Compared to some other proposals: >>>>> > * Compared to DTs, TLVs are OS-independent. >>>> >>>> That's just alternative facts here. Just because Linux uses fdt for >>>> devicetree blobs it is *not* OS dependent. There are several (see >>>> last email) non-Linux users of fdt / libfdt. >>>> >>>> Thanks, >>>> >>>> Moritz >>> >>> It is worth repeating that libdtc is GPL/BSD with the intent of >>> allowing proprietary code to use libdtc. So license shouldn't be a barrier. >>> >>> Using device tree in the header would give us a way of doing enumeration at >>> least for Linux, not sure if that kind of info can be used in Windows >>> in some way. >> >> Actually, enumeration is the only advantage I see with DT. > > Which seems to some point a separate issue to passing in image > specific info such as > encrypted or not, compressed or not or build info metadata. > > So I think in general we can still separate this out into: > - Image specific values > - Reconfiguration specific values > >> Currently I like key/value pairs because they are easily implemented >> and expandable without being rigid in any way. >> >> If we use key/value pairs, we could pass in child device info >> in one of the keys. It could be either a device tree overlay or an >> ACPI overlay. Or could just be left out. So platforms that >> aren't already using DT wouldn't have to. Platforms that >> are have a smooth road to enumeration. > > I'm not sure if you can bundle up enumeration info *with* the image > since you might e.g. > load the same image (i.e. same header) into different FPGAs and the > required update to > the kernel state, i.e. live tree or ACPI would depend entirely on > which FPGA you loaded > the image into w.r.t busses it's connected to etc. > > I do think this info cannot be image specific, but needs to be passed > in via something > external such as a dt overlay. Yes, I think you are right about that. Thanks! Alan > > Cheers, > > Moritz -- 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
> -----Original Message----- > From: Alan Tull [mailto:delicious.quinoa@gmail.com] > Sent: Tuesday, February 21, 2017 10:33 AM > To: Moritz Fischer <moritz.fischer@ettus.com> > Cc: Nadathur, Sundar <sundar.nadathur@intel.com>; Yves Vandervennet > <yves.vandervennet@linux.intel.com>; Jason Gunthorpe > <jgunthorpe@obsidianresearch.com>; matthew.gerlach@linux.intel.com; > linux-kernel <linux-kernel@vger.kernel.org>; linux-fpga@vger.kernel.org; > Marek Vašut <marex@denx.de> > Subject: Re: [RFC 7/8] fpga-region: add sysfs interface > > On Mon, Feb 20, 2017 at 5:49 PM, Moritz Fischer > <moritz.fischer@ettus.com> wrote: > > Hi Alan, > > > > On Sun, Feb 19, 2017 at 3:16 PM, Alan Tull <delicious.quinoa@gmail.com> > wrote: > >> On Sun, Feb 19, 2017 at 9:00 AM, Alan Tull <delicious.quinoa@gmail.com> > wrote: > >>> On Sat, Feb 18, 2017 at 2:45 PM, Moritz Fischer > >>> <moritz.fischer@ettus.com> wrote: > >>>> On Sat, Feb 18, 2017 at 02:10:43PM -0600, Alan Tull wrote: > >>>>> On Sat, Feb 18, 2017 at 6:45 AM, Nadathur, Sundar > >>>>> <sundar.nadathur@intel.com> wrote: > >>>>> > >>>>> > Hi all, > >>>>> > Interesting discussion. The discussion so far has brought out many > concerns such as OS independence. There is an existing format, well-known > to developers, with widespread support, and which is quite extensible: Type- > Length-Value triples. > >>> [...] > >>>> Are there other users of the format? I have to admit I didn't look > >>>> very long, but couldn't find any libs / existing code at a first glance. > >>> > >>> Is there a standard you are looking at? Have you seen any use of > >>> TLV's in the Linux kernel you could point to? Here are some examples of TLVs in the Linux kernel: http://lxr.free-electrons.com/source/net/ipv6/exthdrs.c <-- includes TLV parsing code http://lxr.free-electrons.com/source/drivers/net/ethernet/broadcom/bnx2x/bnx2x_vfpf.c http://lxr.free-electrons.com/source/include/sound/tlv.h In addition, some protocols like LLDP are defined in terms of TLVs. E.g. http://lxr.free-electrons.com/source/drivers/net/ethernet/intel/i40e/i40e_dcb.h?v=4.4 > >> On Sun, Feb 19, 2017 at 9:00 AM, Alan Tull <delicious.quinoa@gmail.com> > wrote: > >>> It is worth repeating that libdtc is GPL/BSD with the intent of > >>> allowing proprietary code to use libdtc. So license shouldn't be a barrier. It is better to check with Windows folks before concluding this. > >> If we use key/value pairs, we could pass in child device info in one > >> of the keys. It could be either a device tree overlay or an ACPI > >> overlay. Or could just be left out. So platforms that aren't > >> already using DT wouldn't have to. Platforms that are have a smooth > >> road to enumeration. > > > > I'm not sure if you can bundle up enumeration info *with* the image > > since you might e.g. > > load the same image (i.e. same header) into different FPGAs and the > > required update to the kernel state, i.e. live tree or ACPI would > > depend entirely on which FPGA you loaded the image into w.r.t busses > > it's connected to etc. > > > > I do think this info cannot be image specific, but needs to be passed > > in via something external such as a dt overlay. > > Yes, I think you are right about that. > > Thanks! > Alan I agree that device enumeration should be separated out from the metadata format considerations. I got some feedback that not everybody may be familiar with TLVs. To make the proposal more clear and specific, let me add more information here. * We represent every datum of interest with its Type (which indicates what it is), a Length (how many bytes it takes) and a Value (its actual value, taking as many bytes as the Length field indicates.) * The exact lengths of the Type and Length fields are up to us, but let us say they are 4 bytes each, for concreteness. As an example, say we want to express the function in the FPGA (crypto, compress, etc.) as a UUID (128 bits long) compliant with RFC 4122. We could have a Type of say 0x00000050 (4 bytes in all) to indicate Function UUIDs, and a Length field of 0x00000010 (16 bytes) and a value of say 3d8814d8-4ecc-4030-8415-0dea4e5e829a . * A Type may indicate that its value is another TLV, thus allowing nested TLVs. The nested TLV may be an array of TLVs (all of same Type) or a structure (TLVs of different Types). With a Key-Value Pair, if the parser comes across an unknown key (such as when an old parser comes across newer metadata), it would not know how to skip that KVP and move onto the next one. With TLVs, that flexibility is built in. Further, we can use bits in the Type field to indicate that it is mandatory, i.e., if the parser does not understand it, it should error out rather than skip it silently. This degree of forward compatibility is difficult to achieve with other formats. Thanks, Sundar
Hi Sundar, On Tue, Feb 21, 2017 at 7:13 PM, Nadathur, Sundar <sundar.nadathur@intel.com> wrote: >> >>> Is there a standard you are looking at? Have you seen any use of >> >>> TLV's in the Linux kernel you could point to? > > Here are some examples of TLVs in the Linux kernel: > http://lxr.free-electrons.com/source/net/ipv6/exthdrs.c <-- includes TLV parsing code > http://lxr.free-electrons.com/source/drivers/net/ethernet/broadcom/bnx2x/bnx2x_vfpf.c > http://lxr.free-electrons.com/source/include/sound/tlv.h > > In addition, some protocols like LLDP are defined in terms of TLVs. E.g. > http://lxr.free-electrons.com/source/drivers/net/ethernet/intel/i40e/i40e_dcb.h?v=4.4 Thanks for the examples, looks interesting. I'll do some reading. >> >> On Sun, Feb 19, 2017 at 9:00 AM, Alan Tull <delicious.quinoa@gmail.com> >> wrote: >> >>> It is worth repeating that libdtc is GPL/BSD with the intent of >> >>> allowing proprietary code to use libdtc. So license shouldn't be a barrier. > It is better to check with Windows folks before concluding this. About whether they can link C code? Or whether BSD licensed code is an issue? > I agree that device enumeration should be separated out from the metadata format considerations. > > I got some feedback that not everybody may be familiar with TLVs. To make the proposal more clear and specific, let me add more information here. > * We represent every datum of interest with its Type (which indicates what it is), a Length (how many bytes it takes) and a Value (its actual value, taking as many bytes as the Length field indicates.) > * The exact lengths of the Type and Length fields are up to us, but let us say they are 4 bytes each, for concreteness. As an example, say we want to express the function in the FPGA (crypto, compress, etc.) as a UUID (128 bits long) compliant with RFC 4122. We could have a Type of say 0x00000050 (4 bytes in all) to indicate Function UUIDs, and a Length field of 0x00000010 (16 bytes) and a value of say 3d8814d8-4ecc-4030-8415-0dea4e5e829a . > * A Type may indicate that its value is another TLV, thus allowing nested TLVs. The nested TLV may be an array of TLVs (all of same Type) or a structure (TLVs of different Types). > > With a Key-Value Pair, if the parser comes across an unknown key (such as when an old parser comes across newer metadata), it would not know how to skip that KVP and move onto the next one. With TLVs, that flexibility is built in. Further, we can use bits in the Type field to indicate that it is mandatory, i.e., if the parser does not understand it, it should error out rather than skip it silently. This degree of forward compatibility is difficult to achieve with other formats. fdt does this out of the box, too. So far I've seen nothing fdt couldn't do (or doesn't do let's rather say). Thanks for clarifying the TLV stuff, Moritz -- 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
On Tue, Feb 21, 2017 at 07:49:19PM -0800, Moritz Fischer wrote: > fdt does this out of the box, too. So far I've seen nothing fdt > couldn't do (or doesn't do let's rather say). tlv/fdt/http headers are all essentially exactly the same thing. Key/value pairs with various encoding schemes. I don't think we don't need a tree of data, so fdt is overkill. tlv is not substantially easier to parse correctly than the structured plain text headers.. It is just in binary so it can represent binary-ish things better. So far the only thing we know we need is a 'bool' for encrypted and a stringish guid thing for partial reconfiguration. The other stuff I've always used is pretty much all textual. 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
Hi all, On Tue, Feb 21, 2017 at 9:12 PM, Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote: > On Tue, Feb 21, 2017 at 07:49:19PM -0800, Moritz Fischer wrote: > >> fdt does this out of the box, too. So far I've seen nothing fdt >> couldn't do (or doesn't do let's rather say). > > tlv/fdt/http headers are all essentially exactly the same > thing. Key/value pairs with various encoding schemes. > > I don't think we don't need a tree of data, so fdt is overkill. > > tlv is not substantially easier to parse correctly than the > structured plain text headers.. It is just in binary so it can > represent binary-ish things better. TLV Seems easy enough. To give an update, I played with fdt a bit to see how far I get in half an hour. I got bool / int / strings to work quite fast (~30mins). Please disregard the horrible hackyness of this ... For simplicity I stuck the header on top of my bitfile with: <snip> /dts-v1/; /{ description = "Test"; compressed = <0>; encrypted = <1>; }; </snip> $ dtc -o header.dtb header.dts $ cat header.dtb mybitfile.bin > /lib/firmware/bitfile_header.bin + static int __fpga_mgr_blob_to_image_info(const void *blob, + struct fpga_image_info *info) + { + int root_offset; + const char *desc; + const uint32_t *_compressed, *_encrypted; + int compressed, encrypted; + + if (fdt_check_header(blob)) { + pr_err("Invalid device tree blob header\n"); + return -EINVAL; + } + + root_offset = fdt_path_offset(blob, "/"); + if (root_offset < 0) { + pr_err("Invalid root offset\n"); + return -EINVAL; + } + + desc = fdt_getprop(blob, root_offset, "description", NULL); + + _compressed = fdt_getprop(blob, root_offset, "compressed", NULL); + if (_compressed) + compressed = fdt32_to_cpu(*_compressed); + + _encrypted = fdt_getprop(blob, root_offset, "encrypted", NULL); + if (_encrypted) + encrypted = fdt32_to_cpu(*_encrypted); + + if (desc) + pr_info("%s: description=%s\n", __func__, desc); + + if (_encrypted && _compressed) + pr_info("%s: compressed? %s encrypted? %s\n", __func__, + compressed ? "Yes" : "No", encrypted ? "Yes" : "No"); + + return 0; + } Which gave me: <snip> [ 19.325182] fpga_manager fpga0: writing bitfile_header.bin to Xilinx Zynq FPGA Manager [ 20.091222] __fpga_mgr_blob_to_image_info: description=Test [ 20.096730] __fpga_mgr_blob_to_image_info: compressed? No encrypted? Yes </snip> So I'm fairly convinced I can make this work, TVLs seem like it could work, too. > So far the only thing we know we need is a 'bool' for encrypted and a > stringish guid thing for partial reconfiguration. Yeah, shouldn't be hard to implement either way. Cheers, Moritz -- 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
On February 21, 2017 9:39 PM, Moritz Fischer wrote: > TLV Seems easy enough. To give an update, I played with fdt a bit to see how > far I get in half an hour. I got bool / int / strings to work quite fast (~30mins). > Please disregard the horrible hackyness of this ... > [...] > So I'm fairly convinced I can make this work, TVLs seem like it could work, > too. > > > So far the only thing we know we need is a 'bool' for encrypted and a > > stringish guid thing for partial reconfiguration. These things have a way of growing beyond their original anticipated needs. > Yeah, shouldn't be hard to implement either way. > > Cheers, > > Moritz Say we upstream a metadata parser. Subsequently, an FPGA image is released with an additional metadata field that the upstreamed version does not handle. How will this be handled if the metadata were in FDT or KVP format? Thanks, Sundar
On Tue, Feb 21, 2017 at 9:46 PM, Nadathur, Sundar <sundar.nadathur@intel.com> wrote: > On February 21, 2017 9:39 PM, Moritz Fischer wrote: > >> TLV Seems easy enough. To give an update, I played with fdt a bit to see how >> far I get in half an hour. I got bool / int / strings to work quite fast (~30mins). >> Please disregard the horrible hackyness of this ... >> [...] >> So I'm fairly convinced I can make this work, TVLs seem like it could work, >> too. >> >> > So far the only thing we know we need is a 'bool' for encrypted and a >> > stringish guid thing for partial reconfiguration. > > These things have a way of growing beyond their original anticipated needs. True. But yeah, not sure about the requirement for a tree, maybe it is overkill. > Say we upstream a metadata parser. Subsequently, an FPGA image is released with an additional metadata field that the upstreamed version does not handle. How will this be handled if the metadata were in FDT or KVP format? The code above will gently ignore it, as I said I spent about half an hour on writing that, just to prove to myself it can be done easily. Logically I don't see anything wrong with ignoring features from the future. But if one insisted one could make a compatibility number part of the required properties I suppose and error out instead. There are examples of optional properties in the devicetree parsing code in the kernel. That being said older drivers / fpga-mgr will not deal with newer features. TLV / KV or whatever doesn't change this fact, or am I missing something? Adding new properties to devicetrees is a well known exercise to cope with newer versions or variations of hardware and happens all the time in the kernel. Older kernels will just ignore them. Thanks, Moritz -- 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
On Tue, Feb 21, 2017 at 11:38 PM, Moritz Fischer <moritz.fischer@ettus.com> wrote: Hi Moritz, > Hi all, > > On Tue, Feb 21, 2017 at 9:12 PM, Jason Gunthorpe > <jgunthorpe@obsidianresearch.com> wrote: >> On Tue, Feb 21, 2017 at 07:49:19PM -0800, Moritz Fischer wrote: >> >>> fdt does this out of the box, too. So far I've seen nothing fdt >>> couldn't do (or doesn't do let's rather say). >> >> tlv/fdt/http headers are all essentially exactly the same >> thing. Key/value pairs with various encoding schemes. >> >> I don't think we don't need a tree of data, so fdt is overkill. >> >> tlv is not substantially easier to parse correctly than the >> structured plain text headers.. It is just in binary so it can >> represent binary-ish things better. > > TLV Seems easy enough. To give an update, I played with fdt a bit to see > how far I get in half an hour. I got bool / int / strings to work > quite fast (~30mins). Thanks for doing this fast piece of exploratory coding. It does confirm that for Linux, using fdt is pretty straightforward here. However... > Please disregard the horrible hackyness of this ... > > For simplicity I stuck the header on top of my bitfile with: > > <snip> > /dts-v1/; > > /{ > description = "Test"; > compressed = <0>; > encrypted = <1>; > }; I understand that this is a simplified example, but it looks a lot like KVP which then gets compiled by dtc. If we do KVP or TLV we get skip using dtc, which would be nice for non-dt OS's using the same images. Also, the license of libfdt allows the use by proprietary os's, but that's not true for dtc. Alan > </snip> > > $ dtc -o header.dtb header.dts > > $ cat header.dtb mybitfile.bin > /lib/firmware/bitfile_header.bin > > + static int __fpga_mgr_blob_to_image_info(const void *blob, > + struct fpga_image_info *info) > + { > + int root_offset; > + const char *desc; > + const uint32_t *_compressed, *_encrypted; > + int compressed, encrypted; > + > + if (fdt_check_header(blob)) { > + pr_err("Invalid device tree blob header\n"); > + return -EINVAL; > + } > + > + root_offset = fdt_path_offset(blob, "/"); > + if (root_offset < 0) { > + pr_err("Invalid root offset\n"); > + return -EINVAL; > + } > + > + desc = fdt_getprop(blob, root_offset, "description", NULL); > + > + _compressed = fdt_getprop(blob, root_offset, "compressed", NULL); > + if (_compressed) > + compressed = fdt32_to_cpu(*_compressed); > + > + _encrypted = fdt_getprop(blob, root_offset, "encrypted", NULL); > + if (_encrypted) > + encrypted = fdt32_to_cpu(*_encrypted); > + > + if (desc) > + pr_info("%s: description=%s\n", __func__, desc); > + > + if (_encrypted && _compressed) > + pr_info("%s: compressed? %s encrypted? %s\n", __func__, > + compressed ? "Yes" : "No", encrypted ? "Yes" : "No"); > + > + return 0; > + } > > Which gave me: > > <snip> > > [ 19.325182] fpga_manager fpga0: writing bitfile_header.bin to > Xilinx Zynq FPGA Manager > [ 20.091222] __fpga_mgr_blob_to_image_info: description=Test > [ 20.096730] __fpga_mgr_blob_to_image_info: compressed? No encrypted? Yes > > </snip> > > So I'm fairly convinced I can make this work, TVLs seem like it could work, too. > >> So far the only thing we know we need is a 'bool' for encrypted and a >> stringish guid thing for partial reconfiguration. > > Yeah, shouldn't be hard to implement either way. > > Cheers, > > Moritz -- 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
On Tue, Feb 21, 2017 at 10:05:42PM -0800, Moritz Fischer wrote: > That being said older drivers / fpga-mgr will not deal with newer features. > TLV / KV or whatever doesn't change this fact, or am I missing something? Often a scheme will have an OPTIONAL and REQUIRED flag for each value. If a REQUIRED value is present but the parser does not support it then the parse fails. For instance, we could mark Encrypted as required, and a zynq driver that does not support the Encrypted tag would not load the bitfile rather than try to load it wrongly. This can be forced into any of the approaches with various levels of hackery. 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
On Wed, Feb 22, 2017 at 8:33 AM, Alan Tull <delicious.quinoa@gmail.com> wrote: > On Tue, Feb 21, 2017 at 11:38 PM, Moritz Fischer > <moritz.fischer@ettus.com> wrote: > > Hi Moritz, > >> Hi all, >> >> On Tue, Feb 21, 2017 at 9:12 PM, Jason Gunthorpe >> <jgunthorpe@obsidianresearch.com> wrote: >>> On Tue, Feb 21, 2017 at 07:49:19PM -0800, Moritz Fischer wrote: >>> >>>> fdt does this out of the box, too. So far I've seen nothing fdt >>>> couldn't do (or doesn't do let's rather say). >>> >>> tlv/fdt/http headers are all essentially exactly the same >>> thing. Key/value pairs with various encoding schemes. >>> >>> I don't think we don't need a tree of data, so fdt is overkill. >>> >>> tlv is not substantially easier to parse correctly than the >>> structured plain text headers.. It is just in binary so it can >>> represent binary-ish things better. >> >> TLV Seems easy enough. To give an update, I played with fdt a bit to see >> how far I get in half an hour. I got bool / int / strings to work >> quite fast (~30mins). > > Thanks for doing this fast piece of exploratory coding. It does > confirm that for Linux, using fdt is pretty straightforward here. > However... > >> Please disregard the horrible hackyness of this ... >> >> For simplicity I stuck the header on top of my bitfile with: >> >> <snip> >> /dts-v1/; >> >> /{ >> description = "Test"; >> compressed = <0>; >> encrypted = <1>; >> }; > > I understand that this is a simplified example, but it looks a lot > like KVP which then gets compiled by dtc. > > If we do KVP or TLV we get skip using dtc, which would be nice for non-dt > OS's using the same images. I used dtc for pure lazyness. Writing a blob to a file using libfdt is about as much code as parsing it. Even with KVP or TLV you have some code that needs to encode / pack your header into a file. libfdt has an example that creates an empty tree. Write that to a file, done. 1: Create empty tree https://github.com/dgibson/dtc/blob/master/libfdt/fdt_empty_tree.c 2: fopen / fwrite, done > Also, the license of libfdt allows the use by proprietary > os's, but that's not true for dtc. Why would that be an issue, you don't need to link anything to run dtc. That being said as I pointed out above you don not have to actually use dtc if the values are known ahead of time (like in our case). What you'd get from using dtc is to encode arbitrary values (for the types supported). Cheers, Moritz -- 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
On Wed, Feb 22, 2017 at 10:44 AM, Moritz Fischer <moritz.fischer@ettus.com> wrote: > On Wed, Feb 22, 2017 at 8:33 AM, Alan Tull <delicious.quinoa@gmail.com> wrote: >> On Tue, Feb 21, 2017 at 11:38 PM, Moritz Fischer >> <moritz.fischer@ettus.com> wrote: >> >> Hi Moritz, >> >>> Hi all, >>> >>> On Tue, Feb 21, 2017 at 9:12 PM, Jason Gunthorpe >>> <jgunthorpe@obsidianresearch.com> wrote: >>>> On Tue, Feb 21, 2017 at 07:49:19PM -0800, Moritz Fischer wrote: >>>> >>>>> fdt does this out of the box, too. So far I've seen nothing fdt >>>>> couldn't do (or doesn't do let's rather say). >>>> >>>> tlv/fdt/http headers are all essentially exactly the same >>>> thing. Key/value pairs with various encoding schemes. >>>> >>>> I don't think we don't need a tree of data, so fdt is overkill. >>>> >>>> tlv is not substantially easier to parse correctly than the >>>> structured plain text headers.. It is just in binary so it can >>>> represent binary-ish things better. >>> >>> TLV Seems easy enough. To give an update, I played with fdt a bit to see >>> how far I get in half an hour. I got bool / int / strings to work >>> quite fast (~30mins). >> >> Thanks for doing this fast piece of exploratory coding. It does >> confirm that for Linux, using fdt is pretty straightforward here. >> However... >> >>> Please disregard the horrible hackyness of this ... >>> >>> For simplicity I stuck the header on top of my bitfile with: >>> >>> <snip> >>> /dts-v1/; >>> >>> /{ >>> description = "Test"; >>> compressed = <0>; >>> encrypted = <1>; >>> }; >> >> I understand that this is a simplified example, but it looks a lot >> like KVP which then gets compiled by dtc. >> >> If we do KVP or TLV we get skip using dtc, which would be nice for non-dt >> OS's using the same images. > > I used dtc for pure lazyness. Writing a blob to a file using libfdt is > about as much > code as parsing it. Thanks for that clarification. I haven't used libfdt myself. That takes care of the license issue I brought up below. I have heard that MS is averse to using DT, but I'm not clear about why other than that it isn't native to Windows already. Alan > Even with KVP or TLV you have some code that needs > to encode / pack your header into a file. > > libfdt has an example that creates an empty tree. Write that to a file, done. > > 1: Create empty tree > https://github.com/dgibson/dtc/blob/master/libfdt/fdt_empty_tree.c > > 2: fopen / fwrite, done > >> Also, the license of libfdt allows the use by proprietary >> os's, but that's not true for dtc. > > Why would that be an issue, you don't need to link anything to run > dtc. That being > said as I pointed out above you don not have to actually use dtc if the values > are known ahead of time (like in our case). What you'd get from using dtc is to > encode arbitrary values (for the types supported). > > Cheers, > > Moritz -- 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
Jason, On Wed, Feb 22, 2017 at 8:44 AM, Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote: > On Tue, Feb 21, 2017 at 10:05:42PM -0800, Moritz Fischer wrote: > >> That being said older drivers / fpga-mgr will not deal with newer features. >> TLV / KV or whatever doesn't change this fact, or am I missing something? > > Often a scheme will have an OPTIONAL and REQUIRED flag for each > value. If a REQUIRED value is present but the parser does not > support it then the parse fails. Oh, of course. I'm a dorque :) Obvioiusly better customer experience like that. > For instance, we could mark Encrypted as required, and a zynq driver > that does not support the Encrypted tag would not load the bitfile > rather than try to load it wrongly. Funny you'd mention that. Thanks to your patch the zynq driver won't ;-) I ran into that while testing, hehe. > This can be forced into any of the approaches with various levels of > hackery. Yeah doesn't seem to hard, Cheers Moritz -- 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
On Wed, Feb 22, 2017 at 09:50:54AM -0800, Moritz Fischer wrote: > > For instance, we could mark Encrypted as required, and a zynq driver > > that does not support the Encrypted tag would not load the bitfile > > rather than try to load it wrongly. > > Funny you'd mention that. Thanks to your patch the zynq driver won't ;-) > I ran into that while testing, hehe. Really? You mean there is no sync word? That really surprises me.. 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
On Wed, Feb 22, 2017 at 9:54 AM, Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote: > On Wed, Feb 22, 2017 at 09:50:54AM -0800, Moritz Fischer wrote: > >> > For instance, we could mark Encrypted as required, and a zynq driver >> > that does not support the Encrypted tag would not load the bitfile >> > rather than try to load it wrongly. >> >> Funny you'd mention that. Thanks to your patch the zynq driver won't ;-) >> I ran into that while testing, hehe. > > Really? You mean there is no sync word? That really surprises me.. Oh wait, that might have been because I didn't drop the header, now that I think about it, it probably still has the sync word. I thought it might have been encrypted (which makes no sense of course ... ) Ignore the noise :D Moritz -- 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
On Tue, Feb 21, 2017 at 12:33 PM, Alan Tull <delicious.quinoa@gmail.com> wrote: > On Mon, Feb 20, 2017 at 5:49 PM, Moritz Fischer > <moritz.fischer@ettus.com> wrote: >> Hi Alan, >> >> On Sun, Feb 19, 2017 at 3:16 PM, Alan Tull <delicious.quinoa@gmail.com> wrote: >>> On Sun, Feb 19, 2017 at 9:00 AM, Alan Tull <delicious.quinoa@gmail.com> wrote: >>>> On Sat, Feb 18, 2017 at 2:45 PM, Moritz Fischer >>>> <moritz.fischer@ettus.com> wrote: >>>>> On Sat, Feb 18, 2017 at 02:10:43PM -0600, Alan Tull wrote: >>>>>> On Sat, Feb 18, 2017 at 6:45 AM, Nadathur, Sundar >>>>>> <sundar.nadathur@intel.com> wrote: >>>>>> >>>>>> > Hi all, >>>>>> > Interesting discussion. The discussion so far has brought out many concerns such as OS independence. There is an existing format, well-known to developers, with widespread support, and which is quite extensible: Type-Length-Value triples. >>>>>> > >>>>>> > To elaborate, a TLV-based format has many advantages: >>>>>> > * It is highly extensible in many ways >>>>>> > -- You can express structures and arrays using TLVs. Our needs right now may seem limited but requirements grow over time. >>>> >>>> Device tree can express arrays. >>>> >>>>>> > -- The space of Type values can be decomposed into standard pre-defined values that are in upstreamed code, and possibly experimental or feature-specific values. >>>>>> > -- Forward compatibility: We can write parsers that can skip unexpected type values, thus allowing old parsers to work with new additions. With some tweaks, old parsers can also reject unexpected values in some ranges while accepting them in other ranges. >>>>>> > * It is OS-independent. >>>>>> > * It can be easily parsed, in kernel or user space. >>>>> >>>>> Are there other users of the format? I have to admit I didn't look very >>>>> long, but couldn't find any libs / existing code at a first glance. >>>> >>>> Is there a standard you are looking at? Have you seen any use of TLV's >>>> in the Linux kernel you could point to? >>>> >>>>> >>>>>> > * It can be validated, in terms of Type values, acceptable lengths, etc. >>>>>> > >>>>>> > It is not directly human-readable but that can be easily addressed with a tool that parses TLVs. >>>>>> > >>>>>> > Compared to some other proposals: >>>>>> > * Compared to DTs, TLVs are OS-independent. >>>>> >>>>> That's just alternative facts here. Just because Linux uses fdt for >>>>> devicetree blobs it is *not* OS dependent. There are several (see >>>>> last email) non-Linux users of fdt / libfdt. >>>>> >>>>> Thanks, >>>>> >>>>> Moritz >>>> >>>> It is worth repeating that libdtc is GPL/BSD with the intent of >>>> allowing proprietary code to use libdtc. So license shouldn't be a barrier. >>>> >>>> Using device tree in the header would give us a way of doing enumeration at >>>> least for Linux, not sure if that kind of info can be used in Windows >>>> in some way. >>> >>> Actually, enumeration is the only advantage I see with DT. >> >> Which seems to some point a separate issue to passing in image >> specific info such as >> encrypted or not, compressed or not or build info metadata. >> >> So I think in general we can still separate this out into: >> - Image specific values >> - Reconfiguration specific values >> >>> Currently I like key/value pairs because they are easily implemented >>> and expandable without being rigid in any way. >>> >>> If we use key/value pairs, we could pass in child device info >>> in one of the keys. It could be either a device tree overlay or an >>> ACPI overlay. Or could just be left out. So platforms that >>> aren't already using DT wouldn't have to. Platforms that >>> are have a smooth road to enumeration. >> >> I'm not sure if you can bundle up enumeration info *with* the image >> since you might e.g. >> load the same image (i.e. same header) into different FPGAs and the >> required update to >> the kernel state, i.e. live tree or ACPI would depend entirely on >> which FPGA you loaded >> the image into w.r.t busses it's connected to etc. >> >> I do think this info cannot be image specific, but needs to be passed >> in via something >> external such as a dt overlay. > > Yes, I think you are right about that. Hi Moritz, Actually now I think that enumeration data in the FPGA image should work even for cases with > 1 FPGA. First case: embedded FPGA. The hardware has one FPGA. The image is designed for a specific board, so there's no problem including the enumeration in the image. Second case: embedded FPGA + a PCIe FPGA. The image will be specific as to whether it goes into the embedded FPGA or the PCIe one. Third case: multiple PCIe FPGAs. The enumeration base will be the PCIe bus of the individual FPGA. If the FPGAs don't have unique pin connections, then the images could go on any of the PCie FPGAs. If there are unique pin connections, then the image will be specific to the FPGA and having the enumeration data in the image is that much more helpful for keeping things straight. Part of the header could specify which specific FPGA it should go on if it is restricted. Of course if the FPGAs have > 1 PR regions, most FPGA architectures do not have relocatable images so those images will be specific for the PR region but not specific to the FPGA except as otherwise noted above. So again, including enumeration data in the bitstream should work unless I'm missing something. What am I missing here? Alan > > Thanks! > Alan > >> >> Cheers, >> >> Moritz -- 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
Alan, On Mon, Feb 27, 2017 at 12:09 PM, Alan Tull <delicious.quinoa@gmail.com> wrote: > First case: embedded FPGA. The hardware has one FPGA. The image is > designed for a specific board, so there's no problem including the > enumeration in the image. Agreed. > Second case: embedded FPGA + a PCIe FPGA. The image will be specific > as to whether it goes into the embedded FPGA or the PCIe one. Agreed. > Third case: multiple PCIe FPGAs. The enumeration base will be the > PCIe bus of the individual FPGA. If the FPGAs don't have unique pin > connections, then the images could go on any of the PCie FPGAs. If > there are unique pin connections, then the image will be specific to > the FPGA and having the enumeration data in the image is that much > more helpful for keeping things straight. Part of the header could > specify which specific FPGA it should go on if it is restricted. Agreed. > Of course if the FPGAs have > 1 PR regions, most FPGA architectures do > not have relocatable images so those images will be specific for the > PR region but not specific to the FPGA except as otherwise noted > above. > > So again, including enumeration data in the bitstream should work > unless I'm missing something. What am I missing here? If you enumeration base is sufficiently smart, I suppose that can work. What you'd probably want is some sort of extension to the platform bus? I really need to take another look at how non-dt systems enumerate to give better feedback on this. Cheers, Moritz -- 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
On Mon, 27 Feb 2017, Moritz Fischer wrote: > Alan, > > On Mon, Feb 27, 2017 at 12:09 PM, Alan Tull <delicious.quinoa@gmail.com> wrote: > > >> First case: embedded FPGA. The hardware has one FPGA. The image is >> designed for a specific board, so there's no problem including the >> enumeration in the image. > > Agreed. > >> Second case: embedded FPGA + a PCIe FPGA. The image will be specific >> as to whether it goes into the embedded FPGA or the PCIe one. > > Agreed. > > >> Third case: multiple PCIe FPGAs. The enumeration base will be the >> PCIe bus of the individual FPGA. If the FPGAs don't have unique pin >> connections, then the images could go on any of the PCie FPGAs. If >> there are unique pin connections, then the image will be specific to >> the FPGA and having the enumeration data in the image is that much >> more helpful for keeping things straight. Part of the header could >> specify which specific FPGA it should go on if it is restricted. > > Agreed. > >> Of course if the FPGAs have > 1 PR regions, most FPGA architectures do >> not have relocatable images so those images will be specific for the >> PR region but not specific to the FPGA except as otherwise noted >> above. >> >> So again, including enumeration data in the bitstream should work >> unless I'm missing something. What am I missing here? > > If you enumeration base is sufficiently smart, I suppose that can work. > What you'd probably want is some sort of extension to the platform bus? I think there is merit it considering the platform bus as part of the overal enumeration strategy. When using device trees, the platform bus is involved. I have also seen other folks enumerate over a platform bus based on data in a format other than device tree. I have experimented with instantiating platform devices from my pcie driver, but I didn't completely work, which could be related to using a fairly old 3.10 kernel. Matthew Gerlach > > I really need to take another look at how non-dt systems enumerate to > give better feedback on this. > > Cheers, > > Moritz > -- > 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 > -- 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
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(®ion->bridge_list); + if (region->get_bridges) + fpga_bridges_put(®ion->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;
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(+)