| Message ID | 20161026225413.GA6220@obsidianresearch.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 27.10.2016 00:54, Jason Gunthorpe wrote: > There is no sense in sending a bitstream we know will not work, and > with the variety of options for bitstream generation in Xilinx tools > it is not terribly clear or very well documented what the correct > input should be, especially since auto-detection was removed from this > driver. > > All Zynq full configuration bitstreams must start with the sync word in > the correct byte order. > > Zynq is also only able to DMA dword quantities, so bitstreams must be > a multiple of 4 bytes. This also fixes a DMA-past the end bug. > > Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> > --- > drivers/fpga/zynq-fpga.c | 25 ++++++++++++++++--------- > 1 file changed, 16 insertions(+), 9 deletions(-) > > diff --git a/drivers/fpga/zynq-fpga.c b/drivers/fpga/zynq-fpga.c > index c2fb4120bd62..46a38772e7ee 100644 > --- a/drivers/fpga/zynq-fpga.c > +++ b/drivers/fpga/zynq-fpga.c > @@ -184,12 +184,26 @@ static int zynq_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags, > > priv = mgr->priv; > > + /* All valid bitstreams are multiples of 32 bits */ > + if ((count % 4) != 0) > + return -EINVAL; > + > err = clk_enable(priv->clk); > if (err) > return err; > > /* don't globally reset PL if we're doing partial reconfig */ > if (!(flags & FPGA_MGR_PARTIAL_RECONFIG)) { > + /* Sanity check the proposed bitstream. It must start with the > + * sync word in the correct byte order and be a multiple of 4 > + * bytes. > + */ > + if (count <= 4 || buf[0] != 0x66 || buf[1] != 0x55 || > + buf[2] != 0x99 || buf[3] != 0xaa) { > + err = -EINVAL; > + goto out_err; > + } I am not quite sure about this and I didn't try it on real hw. But minimum bitstream size 52+B and more likely much more than this. This is taken from u-boot source code and this is full BIN header. The code above is checking only the last word. #define DUMMY_WORD 0xffffffff /* Xilinx binary format header */ static const u32 bin_format[] = { DUMMY_WORD, /* Dummy words */ DUMMY_WORD, DUMMY_WORD, DUMMY_WORD, DUMMY_WORD, DUMMY_WORD, DUMMY_WORD, DUMMY_WORD, 0x000000bb, /* Sync word */ 0x11220044, /* Sync word */ DUMMY_WORD, DUMMY_WORD, 0xaa995566, /* Sync word */ }; Thanks, Michal
On 10/27/2016 12:54 AM, Jason Gunthorpe wrote: > There is no sense in sending a bitstream we know will not work, and > with the variety of options for bitstream generation in Xilinx tools > it is not terribly clear or very well documented what the correct > input should be, especially since auto-detection was removed from this > driver. > > All Zynq full configuration bitstreams must start with the sync word in > the correct byte order. > > Zynq is also only able to DMA dword quantities, so bitstreams must be > a multiple of 4 bytes. This also fixes a DMA-past the end bug. > > Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> > --- > drivers/fpga/zynq-fpga.c | 25 ++++++++++++++++--------- > 1 file changed, 16 insertions(+), 9 deletions(-) > > diff --git a/drivers/fpga/zynq-fpga.c b/drivers/fpga/zynq-fpga.c > index c2fb4120bd62..46a38772e7ee 100644 > --- a/drivers/fpga/zynq-fpga.c > +++ b/drivers/fpga/zynq-fpga.c > @@ -184,12 +184,26 @@ static int zynq_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags, > > priv = mgr->priv; > > + /* All valid bitstreams are multiples of 32 bits */ > + if ((count % 4) != 0) > + return -EINVAL; > + > err = clk_enable(priv->clk); > if (err) > return err; > > /* don't globally reset PL if we're doing partial reconfig */ > if (!(flags & FPGA_MGR_PARTIAL_RECONFIG)) { > + /* Sanity check the proposed bitstream. It must start with the > + * sync word in the correct byte order and be a multiple of 4 > + * bytes. > + */ > + if (count <= 4 || buf[0] != 0x66 || buf[1] != 0x55 || > + buf[2] != 0x99 || buf[3] != 0xaa) { This checks if the bit stream is bigger then 4 bytes. We error out before, if it is smaller. So you should fix the wording in the comment and check for count == 4. Regards, Matthias
On Thu, Oct 27, 2016 at 09:42:03AM +0200, Michal Simek wrote: > I am not quite sure about this and I didn't try it on real hw. > But minimum bitstream size 52+B and more likely much more than this. Oh probably, I didn't try to guess what the minimum size is, that check is just to prevent reading past the end of the buffer. > This is taken from u-boot source code and this is full BIN header. > The code above is checking only the last word. There can be garbage before the sync word. The hardware ignores everything till it gets the sync word. Prior versions of the driver with the autodetection would discard the garbage. Since the autodetection was ripped out I didn't want to search since the intent seems to be for user space to provide a full bitstream, which should start at the sync word, but that is another option. > 0x000000bb, /* Sync word */ > 0x11220044, /* Sync word */ > DUMMY_WORD, > DUMMY_WORD, > 0xaa995566, /* Sync word */ This is the bus width detection pattern, I understood the Zync DevC interface was wired to 32 bits and did not respond to this. Jason
On Thu, Oct 27, 2016 at 10:50:48AM +0200, Matthias Brugger wrote: > >+ /* Sanity check the proposed bitstream. It must start with the > >+ * sync word in the correct byte order and be a multiple of 4 > >+ * bytes. > >+ */ > >+ if (count <= 4 || buf[0] != 0x66 || buf[1] != 0x55 || > >+ buf[2] != 0x99 || buf[3] != 0xaa) { > > This checks if the bit stream is bigger then 4 bytes. We error out before, > if it is smaller. We do? Where? > So you should fix the wording in the comment and check for count == > 4. Ah right, the comment reflected an earlier revision that had the length check here. The count <= 4 should stay here since it is primarily guarding against read past the buffer in the if. Jason
On 10/27/2016 04:39 PM, Jason Gunthorpe wrote: > On Thu, Oct 27, 2016 at 10:50:48AM +0200, Matthias Brugger wrote: >>> + /* Sanity check the proposed bitstream. It must start with the >>> + * sync word in the correct byte order and be a multiple of 4 >>> + * bytes. >>> + */ >>> + if (count <= 4 || buf[0] != 0x66 || buf[1] != 0x55 || >>> + buf[2] != 0x99 || buf[3] != 0xaa) { >> >> This checks if the bit stream is bigger then 4 bytes. We error out before, >> if it is smaller. > > We do? Where? > Just a few lines before: + /* All valid bitstreams are multiples of 32 bits */ + if ((count % 4) != 0) + return -EINVAL; + The only case we don't check is, if count == 0. If we check that here, we can get rid of the count <= 4 check. >> So you should fix the wording in the comment and check for count == >> 4. > > Ah right, the comment reflected an earlier revision that had the > length check here. > > The count <= 4 should stay here since it is primarily guarding against > read past the buffer in the if. If you insist in doing this check, it should be count < 4, because we check the first four elements of buf, or do I miss something? Cheers, Matthias > > Jason > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
On 10/27/2016 12:54 AM, Jason Gunthorpe wrote: > There is no sense in sending a bitstream we know will not work, and > with the variety of options for bitstream generation in Xilinx tools > it is not terribly clear or very well documented what the correct > input should be, especially since auto-detection was removed from this > driver. > > All Zynq full configuration bitstreams must start with the sync word in > the correct byte order. > > Zynq is also only able to DMA dword quantities, so bitstreams must be > a multiple of 4 bytes. This also fixes a DMA-past the end bug. > The you can also fix the transfer_length calculation in zynq_fpga_ops_write, as we don't allow buffers which are not a multiple of 4. Regards, Matthias
On Fri, Oct 28, 2016 at 01:06:08PM +0200, Matthias Brugger wrote: > The only case we don't check is, if count == 0. If we check that here, we > can get rid of the count <= 4 check. You don't think if (count == 0 || buf[3] = 'x') looks weird and wrong? I do. > >The count <= 4 should stay here since it is primarily guarding against > >read past the buffer in the if. > > If you insist in doing this check, it should be count < 4, because we check > the first four elements of buf, or do I miss something? count = 4 and count = 0 are both invalid. A bitstream consisting of only the sync word is also going to fail programming. As Michal said, the actual min bitstream length is probably >> 50 bytes Jason
On Fri, Oct 28, 2016 at 01:12:06PM +0200, Matthias Brugger wrote: > >Zynq is also only able to DMA dword quantities, so bitstreams must be > >a multiple of 4 bytes. This also fixes a DMA-past the end bug. > > The you can also fix the transfer_length calculation in zynq_fpga_ops_write, > as we don't allow buffers which are not a multiple of 4. Didn't I do that? Did you see something else to change in the dma part? Jason
On 28/10/16 17:48, Jason Gunthorpe wrote: > On Fri, Oct 28, 2016 at 01:12:06PM +0200, Matthias Brugger wrote: >>> Zynq is also only able to DMA dword quantities, so bitstreams must be >>> a multiple of 4 bytes. This also fixes a DMA-past the end bug. >> >> The you can also fix the transfer_length calculation in zynq_fpga_ops_write, >> as we don't allow buffers which are not a multiple of 4. > > Didn't I do that? Did you see something else to change in the dma > part? > Sure you did, my mistake. Matthias
diff --git a/drivers/fpga/zynq-fpga.c b/drivers/fpga/zynq-fpga.c index c2fb4120bd62..46a38772e7ee 100644 --- a/drivers/fpga/zynq-fpga.c +++ b/drivers/fpga/zynq-fpga.c @@ -184,12 +184,26 @@ static int zynq_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags, priv = mgr->priv; + /* All valid bitstreams are multiples of 32 bits */ + if ((count % 4) != 0) + return -EINVAL; + err = clk_enable(priv->clk); if (err) return err; /* don't globally reset PL if we're doing partial reconfig */ if (!(flags & FPGA_MGR_PARTIAL_RECONFIG)) { + /* Sanity check the proposed bitstream. It must start with the + * sync word in the correct byte order and be a multiple of 4 + * bytes. + */ + if (count <= 4 || buf[0] != 0x66 || buf[1] != 0x55 || + buf[2] != 0x99 || buf[3] != 0xaa) { + err = -EINVAL; + goto out_err; + } + /* assert AXI interface resets */ regmap_write(priv->slcr, SLCR_FPGA_RST_CTRL_OFFSET, FPGA_RST_ALL_MASK); @@ -287,12 +301,9 @@ static int zynq_fpga_ops_write(struct fpga_manager *mgr, struct zynq_fpga_priv *priv; int err; char *kbuf; - size_t in_count; dma_addr_t dma_addr; - u32 transfer_length; u32 intr_status; - in_count = count; priv = mgr->priv; kbuf = dma_alloc_coherent(priv->dev, count, &dma_addr, GFP_KERNEL); @@ -318,11 +329,7 @@ static int zynq_fpga_ops_write(struct fpga_manager *mgr, */ zynq_fpga_write(priv, DMA_SRC_ADDR_OFFSET, (u32)(dma_addr) + 1); zynq_fpga_write(priv, DMA_DST_ADDR_OFFSET, (u32)DMA_INVALID_ADDRESS); - - /* convert #bytes to #words */ - transfer_length = (count + 3) / 4; - - zynq_fpga_write(priv, DMA_SRC_LEN_OFFSET, transfer_length); + zynq_fpga_write(priv, DMA_SRC_LEN_OFFSET, count / 4); zynq_fpga_write(priv, DMA_DEST_LEN_OFFSET, 0); wait_for_completion(&priv->dma_done); @@ -338,7 +345,7 @@ static int zynq_fpga_ops_write(struct fpga_manager *mgr, clk_disable(priv->clk); out_free: - dma_free_coherent(priv->dev, in_count, kbuf, dma_addr); + dma_free_coherent(priv->dev, count, kbuf, dma_addr); return err; }
There is no sense in sending a bitstream we know will not work, and with the variety of options for bitstream generation in Xilinx tools it is not terribly clear or very well documented what the correct input should be, especially since auto-detection was removed from this driver. All Zynq full configuration bitstreams must start with the sync word in the correct byte order. Zynq is also only able to DMA dword quantities, so bitstreams must be a multiple of 4 bytes. This also fixes a DMA-past the end bug. Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> --- drivers/fpga/zynq-fpga.c | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-)