fpga zynq: Check the bitstream for validity
Subsystems (2)
PATCH 1/1 fpga zynq: Check the bitstream for validity
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.
--- drivers/fpga/zynq-fpga.c | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-)
drivers/fpga/zynq-fpga.c +16 -9
@@ -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; }
Signature · 1 line
2.1.4
PATCH 1/1 fpga zynq: Check the bitstream for validity
Quoted 34 lines · Jason Gunthorpe
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 <[email protected]>--- drivers/fpga/zynq-fpga.c | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-)drivers/fpga/zynq-fpga.c +4
-0@@ -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 */
drivers/fpga/zynq-fpga.c static int zynq_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags, · line 205
Quoted 10 lines
drivers/fpga/zynq-fpga.c +9
-0if (!(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
PATCH 1/1 fpga zynq: Check the bitstream for validity
Quoted 2 lines · Michal Simek
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.
Quoted 2 lines
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.
Quoted 5 lines
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
PATCH 1/1 fpga zynq: Check the bitstream for validity
In reply to Jason GunthorpeQuoted 31 lines · Jason Gunthorpe
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 <[email protected]>--- drivers/fpga/zynq-fpga.c | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-)drivers/fpga/zynq-fpga.c +4
-0@@ -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)
drivers/fpga/zynq-fpga.c static int zynq_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags, · line 202
Quoted 10 lines
drivers/fpga/zynq-fpga.c +6
-0return 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
PATCH 1/1 fpga zynq: Check the bitstream for validity
Quoted 9 lines · Matthias Brugger
drivers/fpga/zynq-fpga.c static int zynq_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags, · line 202
Quoted 6 lines
drivers/fpga/zynq-fpga.c +6
-0+/* 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?
Quoted 2 lines
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
PATCH 1/1 fpga zynq: Check the bitstream for validity
Quoted 11 lines · Jason Gunthorpe
Quoted 9 lines · Matthias Brugger
drivers/fpga/zynq-fpga.c static int zynq_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags, · line 202
Quoted 6 lines
drivers/fpga/zynq-fpga.c +6
-0+/* 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.
Quoted 2 lines
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.
Quoted 8 lines
Quoted 2 lines
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
Quoted 8 lines
Jason
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
PATCH 1/1 fpga zynq: Check the bitstream for validity
Quoted 2 lines · Matthias Brugger
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.
Quoted 5 lines
Quoted 2 lines
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
PATCH 1/1 fpga zynq: Check the bitstream for validity
Quoted 11 lines · Jason Gunthorpe
Quoted 2 lines · Matthias Brugger
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.
That wasn't what I meant. Apart it looks quite wrong, because when the count is zero buf[3] points to anything but a valid value.
Quoted 11 lines
Quoted 5 lines
Quoted 2 lines
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
Sure but we are checking here that the bitstream passed to the kernel is correct. I was thinking of something like:
drivers/fpga/zynq-fpga.c +0 -0
@@ -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 || (count % 4) != 0)
+ return -EINVAL;
+
PATCH 1/1 fpga zynq: Check the bitstream for validity
Quoted 2 lines · Matthias Brugger
Sure but we are checking here that the bitstream passed to the kernel is correct.
The intent to check if it *possible* that the bitstream is correct. Correct means that DONE will assert after programming. A 4 byte bitstream will never assert DONE.
Arguably the threshold should be larger but we don't know what the true minimum is.
Review
Quoted 4 lines
(quoted diff) +4
-0+ /* All valid bitstreams are multiples of 32 bits */ + if (!count || (count % 4) != 0) + return -EINVAL; +
Too clever for my taste.
Aside from this, is the general idea even OK? In my world I cannonize the bitstream to start with the sync word, but others may not be doing that.
I designed this patch based on the prior work to remove the auto-detection, eg that the driver should be passed a canonized bitstream.
The problem with the way it is now is how hard it is to figure out what the right bitstream format should be. Clear instructions to canonize by droping the header before the sync word and byteswap so the sync word is in the given order is much clearer..
Jason
PATCH 1/1 fpga zynq: Check the bitstream for validity
Quoted 19 lines · Jason Gunthorpe
Quoted 2 lines · Matthias Brugger
Sure but we are checking here that the bitstream passed to the kernel is correct.
The intent to check if it *possible* that the bitstream is correct. Correct means that DONE will assert after programming. A 4 byte bitstream will never assert DONE.
Arguably the threshold should be larger but we don't know what the true minimum is.
Review
Quoted 4 lines
(quoted diff) +4
-0+ /* All valid bitstreams are multiples of 32 bits */ + if (!count || (count % 4) != 0) + return -EINVAL; +Too clever for my taste.
Quoted 7 lines
Aside from this, is the general idea even OK? In my world I cannonize the bitstream to start with the sync word, but others may not be doing that.
I designed this patch based on the prior work to remove the auto-detection, eg that the driver should be passed a canonized bitstream.
I'm fine with checking for boundary cases where the bitstreams are obviously too small or wrong size, I'm not convinced that checking using internal knowledge about the bistream format inside the kernel is the right place.
Quoted 4 lines
The problem with the way it is now is how hard it is to figure out what the right bitstream format should be. Clear instructions to canonize by droping the header before the sync word and byteswap so the sync word is in the given order is much clearer..
I don't know about you, but for my designs I can just use what drops out of my Vivado build. Are you unhappy with the way we document which format to use, or that we don't slice off the beginning (that gets ignored by hardware?).
Thanks for addressing the issues with the length calculations,
Moritz
PATCH 1/1 fpga zynq: Check the bitstream for validity
Quoted 4 lines · Moritz Fischer
I'm fine with checking for boundary cases where the bitstreams are obviously too small or wrong size, I'm not convinced that checking using internal knowledge about the bistream format inside the kernel is the right place.
To be clear, the sync word is documented in the Xilinx public docs, it is mandatory. I don't think there is anything wrong with doing basic validation on the structure of the bitstream before sending it.
Quoted 7 lines
Quoted 4 lines
The problem with the way it is now is how hard it is to figure out what the right bitstream format should be. Clear instructions to canonize by droping the header before the sync word and byteswap so the sync word is in the given order is much clearer..
I don't know about you, but for my designs I can just use what drops out of my Vivado build.
Are you sure? With a 4.8 kernel?
# (in vivado 2016.3) write_bitstream -bin_file foo
$ hexdump -C foo.bin 00000000 ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................| * 00000020 00 00 00 bb 11 22 00 44 ff ff ff ff ff ff ff ff |.....".D........| 00000030 aa 99 55 66 20 00 00 00 30 02 20 01 00 00 00 00 |..Uf ...0. .....|
So that isn't going to work, it needs byte swapping
$ hexdump -C foo.bit 000000a0 bb 11 22 00 44 ff ff ff ff ff ff ff ff aa 99 55 |..".D..........U| 000000b0 66 20 00 00 00 30 02 20 01 00 00 00 00 30 02 00 |f ...0. .....0..|
This also is not going to work, it needs byteswapping and the sync word has to be 4 byte aligned.
What did you do to get a working bitfile? Are you using one of the Vivado automatic flows that 'handles' this for you? I am not.
Remember, 4.8 now has the patch to remove the autodetection that used to correct both of the above two problems..
So from my perspective, this driver is incompatible with the output of the Xilinx tools. I don't really care, we always post-process the output of write_bitfile, and I am happy to provide a canonized bitstream, but the driver needs to do more to help people get this right.
Quoted 2 lines
Are you unhappy with the way we document which format to use, or that we don't slice off the beginning (that gets ignored by hardware?).
Well, I'm unhappy I spent an hour wondering why things didn't work with no information on what the expected format was now for this driver. For a bit I wondered if there was a driver bug as this stuff all worked for me with the original xdevcfg driver.
Some of the problems were bugs on my part (which would have been found much faster with validation), but at the end of the day this is horribly unfriendly. You get a timeout and a 'Failed' message, thats it.
I think all common bitstream formatting errors would be detected by simply validating the sync word.
Jason
PATCH 1/1 fpga zynq: Check the bitstream for validity
Jason,
Quoted 10 lines · Jason Gunthorpe
Quoted 4 lines · Moritz Fischer
I'm fine with checking for boundary cases where the bitstreams are obviously too small or wrong size, I'm not convinced that checking using internal knowledge about the bistream format inside the kernel is the right place.
To be clear, the sync word is documented in the Xilinx public docs, it is mandatory. I don't think there is anything wrong with doing basic validation on the structure of the bitstream before sending it.
By 'internal' I meant internal to the bitstream. On second thought, you might have a point with the error message being not helpful (see below).
Quoted 24 lines
Quoted 8 lines
Quoted 5 lines
The problem with the way it is now is how hard it is to figure out what the right bitstream format should be. Clear instructions to canonize by droping the header before the sync word and byteswap so the sync word is in the given order is much clearer..
I don't know about you, but for my designs I can just use what drops out of my Vivado build.
Are you sure? With a 4.8 kernel?
# (in vivado 2016.3)
write_bitstream-bin_file foo$ hexdump -C foo.bin 00000000 ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................| * 00000020 00 00 00 bb 11 22 00 44 ff ff ff ff ff ff ff ff |.....".D........| 00000030 aa 99 55 66 20 00 00 00 30 02 20 01 00 00 00 00 |..Uf ...0. .....|So that isn't going to work, it needs byte swapping
$ hexdump -C foo.bit 000000a0 bb 11 22 00 44 ff ff ff ff ff ff ff ff aa 99 55 |..".D..........U| 000000b0 66 20 00 00 00 30 02 20 01 00 00 00 00 30 02 00 |f ...0. .....0..|
Quoted 5 lines
This also is not going to work, it needs byteswapping and the sync word has to be 4 byte aligned.
What did you do to get a working bitfile? Are you using one of the Vivado automatic flows that 'handles' this for you? I am not.
https://github.com/EttusResearch/fpgadev/blob/master/usrp3/tools/scripts/viv_utils.tcl#L165
Is what our build process does (we set the byte_swap_bin parameter to 1). You're right in that write_bitstream will give you a non-swapped version.
Quoted 2 lines
Remember, 4.8 now has the patch to remove the autodetection that used to correct both of the above two problems..
The intial version had all the 'autocorrection' stuff in there, it was then decided that we're not gonna support this.
The fact that we ended up supporting .bin in the initial version, and then had a 'patch' to remove that was due to Greg pulling in my code too early (before I could resubmit a squashed version).
If we reevaluate now that we wanna support swapping we should do this at a framework level. Maybe a preprocess callback or a FPGA_MGR_SWAP flag. I'll need to think about this :-)
Quoted 5 lines
So from my perspective, this driver is incompatible with the output of the Xilinx tools. I don't really care, we always post-process the output of
write_bitfile, and I am happy to provide a canonized bitstream, but the driver needs to do more to help people get this right.
Ok, so I'm fine with adding the checks and a warning if you don't find the sync word. We could add documentation describing which format we expect.
Quoted 9 lines
Quoted 3 lines
Are you unhappy with the way we document which format to use, or that we don't slice off the beginning (that gets ignored by hardware?).
Well, I'm unhappy I spent an hour wondering why things didn't work with no information on what the expected format was now for this driver. For a bit I wondered if there was a driver bug as this stuff all worked for me with the original xdevcfg driver.
Quoted 7 lines
Some of the problems were bugs on my part (which would have been found much faster with validation), but at the end of the day this is horribly unfriendly. You get a timeout and a 'Failed' message, thats it.
I think all common bitstream formatting errors would be detected by simply validating the sync word.
Ok. That sounds reasonable.
Cheers
Moritz
PATCH 1/1 fpga zynq: Check the bitstream for validity
Quoted 4 lines · Moritz Fischer
Quoted 2 lines
What did you do to get a working bitfile? Are you using one of the Vivado automatic flows that 'handles' this for you? I am not.
https://github.com/EttusResearch/fpgadev/blob/master/usrp3/tools/scripts/viv_utils.tcl#L165
Hm 404
Quoted 3 lines
Is what our build process does (we set the
byte_swap_binparameter to 1). You're right in thatwrite_bitstreamwill give you a non-swapped version.
?? byte_swap_bin is not documented in UG835
Quoted 3 lines
If we reevaluate now that we wanna support swapping we should do this at a framework level. Maybe a preprocess callback or a
FPGA_MGR_SWAPflag. I'll need to think about this :-)
I'm perfectly fine with the driver only working with a single canonical bitfile format. That seems completly reasonable, and I prefer an efficient programming sequence for performance.
Further, eventually this framework is going to have to be fixed to be able to DMA out of the page cache and in that world there is no sensible option to process the data before sending it on to the hardware.
Quoted 9 lines
Quoted 5 lines
So from my perspective, this driver is incompatible with the output of the Xilinx tools. I don't really care, we always post-process the output of
write_bitfile, and I am happy to provide a canonized bitstream, but the driver needs to do more to help people get this right.Ok, so I'm fine with adding the checks and a warning if you don't find the sync word. We could add documentation describing which format we expect.
Maybe you could send a patch to update the comments for the driver, or add a documentation file how to produce an acceptable format using Xilinx tools..
Quoted 1 line
Ok. That sounds reasonable.
So, the question still remains, should the driver require the header be stripped (eg the sync word is the first 4 bytes), or should it search the first bit for an aligned sync word?
Either requirement is acceptable to the hardware. My patch does the former, I suspect you need the later?
Jason
PATCH 1/1 fpga zynq: Check the bitstream for validity
Jason,
Quoted 8 lines · Jason Gunthorpe
Quoted 4 lines · Moritz Fischer
Quoted 2 lines
What did you do to get a working bitfile? Are you using one of the Vivado automatic flows that 'handles' this for you? I am not.
https://github.com/EttusResearch/fpgadev/blob/master/usrp3/tools/scripts/viv_utils.tcl#L165
Hm 404
Whooopsie ... that was the internal link. Try that one:
https://github.com/EttusResearch/fpga/blob/master/usrp3/tools/scripts/viv_utils.tcl#L165
It's not a single command but rather a sequence of steps we take to create an image that works (using write_cfgmem instead of write_binfile)
Quoted 26 lines
Quoted 4 lines
Is what our build process does (we set the
byte_swap_binparameter to 1). You're right in thatwrite_bitstreamwill give you a non-swapped version.??
byte_swap_binis not documented in UG835Quoted 3 lines
If we reevaluate now that we wanna support swapping we should do this at a framework level. Maybe a preprocess callback or a
FPGA_MGR_SWAPflag. I'll need to think about this :-)I'm perfectly fine with the driver only working with a single canonical bitfile format. That seems completly reasonable, and I prefer an efficient programming sequence for performance.
Further, eventually this framework is going to have to be fixed to be able to DMA out of the page cache and in that world there is no sensible option to process the data before sending it on to the hardware.
Quoted 6 lines
Quoted 5 lines
So from my perspective, this driver is incompatible with the output of the Xilinx tools. I don't really care, we always post-process the output of
write_bitfile, and I am happy to provide a canonized bitstream, but the driver needs to do more to help people get this right.
Quoted 7 lines
Quoted 3 lines
Ok, so I'm fine with adding the checks and a warning if you don't find the sync word. We could add documentation describing which format we expect.
Maybe you could send a patch to update the comments for the driver, or add a documentation file how to produce an acceptable format using Xilinx tools..
Yeah will do. I don't know if the generation flow outlined above is perfect, we just pad our images and I haven't run into issues so far.
Quoted 3 lines
So, the question still remains, should the driver require the header be stripped (eg the sync word is the first 4 bytes), or should it search the first bit for an aligned sync word?
So currently we don't require it to be stripped, changing it so it does require stripping would break people's setups that already use the current implementation.
That being said, I don't like the idea of the driver having to search either...
Michal, S?ren you guys have a preference / input?
Quoted 2 lines
Either requirement is acceptable to the hardware. My patch does the former, I suspect you need the later?
For my usecases I could deal with either way, looking at backwards compat the latter one would be preferential I supose ...
Cheers,
Moritz
PATCH 1/1 fpga zynq: Check the bitstream for validity
Quoted 2 lines · Moritz Fischer
It's not a single command but rather a sequence of steps we take to create an image that works (using
write_cfgmeminstead ofwrite_binfile)
Ah, and it relies on newer Vivado features too.. Never had a use for write_cfgmem before, and wouldn't have thought it did this.
I always do these transform externally and we tack our own header onto the bitfile as well..
Quoted 7 lines
Quoted 3 lines
So, the question still remains, should the driver require the header be stripped (eg the sync word is the first 4 bytes), or should it search the first bit for an aligned sync word?
So currently we don't require it to be stripped, changing it so it does require stripping would break people's setups that already use the current implementation.
Considering there is a way to produce an acceptable bitfile via write_cfgmem I think we should stick with that and allow the header to be present, otherwise all users need yet another tool.
I'll send another patch when I get back from the plumbers conference.
Quoted 5 lines
Quoted 2 lines
Either requirement is acceptable to the hardware. My patch does the former, I suspect you need the later?
For my usecases I could deal with either way, looking at backwards compat the latter one would be preferential I supose ...
Well, there are no in-kernel users, and no uapi, so it isn't such a big deal. I'm actually surprised this got merged without users ..
Jason
PATCH 1/1 fpga zynq: Check the bitstream for validity
Hi Jason,
Quoted 24 lines · Jason Gunthorpe
Quoted 2 lines · Moritz Fischer
It's not a single command but rather a sequence of steps we take to create an image that works (using
write_cfgmeminstead ofwrite_binfile)Ah, and it relies on newer Vivado features too.. Never had a use for
write_cfgmembefore, and wouldn't have thought it did this.I always do these transform externally and we tack our own header onto the bitfile as well..
Quoted 7 lines
Quoted 3 lines
So, the question still remains, should the driver require the header be stripped (eg the sync word is the first 4 bytes), or should it search the first bit for an aligned sync word?
So currently we don't require it to be stripped, changing it so it does require stripping would break people's setups that already use the current implementation.
Considering there is a way to produce an acceptable bitfile via
write_cfgmemI think we should stick with that and allow the header to be present, otherwise all users need yet another tool.I'll send another patch when I get back from the plumbers conference.
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
Either requirement is acceptable to the hardware. My patch does the former, I suspect you need the later?
For my usecases I could deal with either way, looking at backwards compat the latter one would be preferential I supose ...
Well, there are no in-kernel users, and no uapi, so it isn't such a big deal. I'm actually surprised this got merged without users ..
There were several things in this whole thread.
Regarding BIT and BIN format. This support is in vivado for a long time and it is up2you what you want to support. We have removed that BIT support and not doing any swap by saying only BIN format is supported.
If driver check header and if it is not valid you should just error out. If header is fine driver can program it.
Regarding what you need to do in Vivado to get your bitstream right is completely out of this thread and TBH I don't know it too.
Thanks,
Michal
PATCH 1/1 fpga zynq: Check the bitstream for validity
Quoted 3 lines · Michal Simek
Regarding BIT and BIN format. This support is in vivado for a long time and it is up2you what you want to support. We have removed that BIT support and not doing any swap by saying only BIN format is supported.
BIN is not supported, it needs a swap as well.
Moritz has it right, you have to use vivado to create a PROM image to be compatible with the driver.
Jason
PATCH 1/1 fpga zynq: Check the bitstream for validity
Quoted 10 lines · Jason Gunthorpe
Quoted 3 lines · Michal Simek
Regarding BIT and BIN format. This support is in vivado for a long time and it is up2you what you want to support. We have removed that BIT support and not doing any swap by saying only BIN format is supported.
BIN is not supported, it needs a swap as well.
Moritz has it right, you have to use vivado to create a PROM image to be compatible with the driver.
hm than that's bad.
Thanks,
Michal
PATCH 1/1 fpga zynq: Check the bitstream for validity
Quoted 7 lines · Michal Simek
Quoted 6 lines · Jason Gunthorpe
Quoted 3 lines · Michal Simek
Regarding BIT and BIN format. This support is in vivado for a long time and it is up2you what you want to support. We have removed that BIT support and not doing any swap by saying only BIN format is supported.
Quoted 6 lines
Quoted 4 lines
BIN is not supported, it needs a swap as well.
Moritz has it right, you have to use vivado to create a PROM image to be compatible with the driver.
hm than that's bad.
IMHO, Xilinx made an error with Zynq DevC, the DMA does not accept a memory image that is output by the usual Xilinx tools. It should have accepted a byte swapped input.
I think Moritz is right, the fpgamgr *should not* alter the bitstream in any way. This is important for future work to make the DMA do gather and avoid the really bad high-order allocation.
So users will have to provide byte swapped .bin files - the vivado write_cfgmem command will produce them - this all needs to be documented.
Also, I think Punnaiah (?) was telling me that bitstream encryption does not work - DevC must be told the bitstream is encrypted. That seems like something that needs work at the fpgamgr level - and maybe this driver should auto-detect encryption by looking at the bitfile (as is typical for Xilinx programming)
Jason
PATCH 1/1 fpga zynq: Check the bitstream for validity
Quoted 27 lines · Jason Gunthorpe
Quoted 7 lines · Michal Simek
Quoted 6 lines · Jason Gunthorpe
Quoted 3 lines · Michal Simek
Regarding BIT and BIN format. This support is in vivado for a long time and it is up2you what you want to support. We have removed that BIT support and not doing any swap by saying only BIN format is supported.
Quoted 6 lines
Quoted 4 lines
BIN is not supported, it needs a swap as well.
Moritz has it right, you have to use vivado to create a PROM image to be compatible with the driver.
hm than that's bad.
IMHO, Xilinx made an error with Zynq DevC, the DMA does not accept a memory image that is output by the usual Xilinx tools. It should have accepted a byte swapped input.
I think Moritz is right, the fpgamgr *should not* alter the bitstream in any way. This is important for future work to make the DMA do gather and avoid the really bad high-order allocation.
So users will have to provide byte swapped .bin files - the vivado
write_cfgmemcommand will produce them - this all needs to be documented.
Quoted 6 lines
Also, I think Punnaiah (?) was telling me that bitstream encryption does not work - DevC must be told the bitstream is encrypted. That seems like something that needs work at the fpgamgr level - and maybe this driver should auto-detect encryption by looking at the bitfile (as is typical for Xilinx programming)
I think the basic idea behind the commit is flawed to begin with and the patch should be discarded completely. The same discussion was done for the Xilinx FPGA manager driver, which resulted in the decision that the tooling should create a proper file. This driver should either become obsolete or at least move into the same direction as the FPGA manager rather than away from that.
Besides political arguments, there's a more pressing technical argument against this theck as well: The whole check is pointless since the hardware itself already verifies the validity of the stream. Sending bitstreams intended for other devices has no effect at all. Even sending random data doesn't have any effect, the hardware will discard it. There's no reason to waste CPU cycles duplicating this check in software.
Kind regards,
Mike Looijmans
System Expert
TOPIC Products
Materiaalweg 4, NL-5681 RJ Best
Postbus 440, NL-5680 AK Best
Telefoon: +31 (0) 499 33 69 79
E-mail: mike.looijmans at topicproducts.com
Website: www.topicproducts.com
Please consider the environment before printing this e-mail
PATCH 1/1 fpga zynq: Check the bitstream for validity
Quoted 44 lines · Mike Looijmans
Quoted 29 lines · Jason Gunthorpe
Quoted 8 lines · Michal Simek
Quoted 7 lines · Jason Gunthorpe
Quoted 4 lines · Michal Simek
Regarding BIT and BIN format. This support is in vivado for a long time
and it is up2you what you want to support. We have removed that BIT support and not doing any swap by saying only BIN format is supported.Quoted 7 lines
Quoted 5 lines
BIN is not supported, it needs a swap as well.
Moritz has it right, you have to use vivado to create a PROM image to be
compatible with the driver.hm than that's bad.
IMHO, Xilinx made an error with Zynq DevC, the DMA does not accept a memory image that is output by the usual Xilinx tools. It should have accepted a byte swapped input.
I think Moritz is right, the fpgamgr *should not* alter the bitstream in any way. This is important for future work to make the DMA do gather and avoid the really bad high-order allocation.
So users will have to provide byte swapped .bin files - the vivado
write_cfgmemcommand will produce them - this all needs to be documented.Quoted 6 lines
Also, I think Punnaiah (?) was telling me that bitstream encryption does not work - DevC must be told the bitstream is encrypted. That seems like something that needs work at the fpgamgr level - and maybe this driver should auto-detect encryption by looking at the bitfile (as is typical for Xilinx programming)
I think the basic idea behind the commit is flawed to begin with and the patch should be discarded completely. The same discussion was done for the Xilinx FPGA manager driver, which resulted in the decision that the tooling should create a proper file. This driver should either become obsolete or at least move into the same direction as the FPGA manager rather than away from that.
Quoted 7 lines
Besides political arguments, there's a more pressing technical argument against this theck as well: The whole check is pointless since the hardware itself already verifies the validity of the stream. Sending bitstreams intended for other devices has no effect at all. Even sending random data doesn't have any effect, the hardware will discard it. There's no reason to waste CPU cycles duplicating this check in software.
I get your point. Especially looping to the whole file to find the sync header can take a while, especially if the file is big and the sync header is not present.
So I think the whole idea behind this patch is to provide feedback to the user about what went wrong when trying to update the FPGA. Is there a way to get this information from the hardware which discards the update?
Regards,
Matthias
PATCH 1/1 fpga zynq: Check the bitstream for validity
Quoted 3 lines · Matthias Brugger
I get your point. Especially looping to the whole file to find the sync header can take a while, especially if the file is big and the sync header is not present.
Er, no not at all. If you send garbage to the FPGA the driver detects it after a 2.5s timeout. The memory scan is always alot faster.
In the normal success case it is ~5 compares.
Quoted 3 lines
So I think the whole idea behind this patch is to provide feedback to the user about what went wrong when trying to update the FPGA. Is there a way to get this information from the hardware which discards the update?
No, not with such specificity.
Jason
PATCH 1/1 fpga zynq: Check the bitstream for validity
In reply to Mike LooijmansQuoted 4 lines · Mike Looijmans
I think the basic idea behind the commit is flawed to begin with and the patch should be discarded completely. The same discussion was done for the Xilinx FPGA manager driver, which resulted in the decision that the tooling should create a proper file.
That hasn't changed at all, this just produces a clear and obvious message about what is wrong instead of 'timed out'.
Remember, again, the Xilinx tools do not produce an acceptable bitstream file by default. You need to do very strange and special things to get something acceptable to this driver. It is a very easy mistake to make and hard to track down if you don't already know these details.
Quoted 2 lines
This driver should either become obsolete or at least move into the same direction as the FPGA manager rather than away from that.
I don't understand what you are talking about here, this is a fpga mgr driver already, and is consistent with the FPGA manger - the auto detect stuff was removed a while ago and isn't coming back.
It is perfectly reasonable for fpga manager drivers to pre-validate the proposed bitstream, IMHO all of the drivers should try and do that step.
Remember, it is deeply unlikely but sending garbage to an FPGA could damage it.
Quoted 3 lines
Besides political arguments, there's a more pressing technical argument against this theck as well: The whole check is pointless since the hardware itself already verifies the validity of the stream.
The purpose of the check is not to protect the hardware. The check is to help the user provide the correct input because the hardware provides no feedback at all on what is wrong with the input.
And again, the out-of-tree Xilinx driver accepted files that this driver does not, so having a clear and understandable message is going to be very important for users.
Quoted 2 lines
doesn't have any effect, the hardware will discard it. There's no reason to waste CPU cycles duplicating this check in software.
In the typical success case we are talking about 5 32 bit compares, which isn't even worth considering.
Jason
PATCH 1/1 fpga zynq: Check the bitstream for validity
Quoted 15 lines · Jason Gunthorpe
Quoted 4 lines · Mike Looijmans
I think the basic idea behind the commit is flawed to begin with and the patch should be discarded completely. The same discussion was done for the Xilinx FPGA manager driver, which resulted in the decision that the tooling should create a proper file.
That hasn't changed at all, this just produces a clear and obvious message about what is wrong instead of 'timed out'.
Remember, again, the Xilinx tools do not produce an acceptable bitstream file by default. You need to do very strange and special things to get something acceptable to this driver. It is a very easy mistake to make and hard to track down if you don't already know these details.
Quoted 6 lines
Quoted 2 lines
This driver should either become obsolete or at least move into the same direction as the FPGA manager rather than away from that.
I don't understand what you are talking about here, this is a fpga mgr driver already, and is consistent with the FPGA manger - the auto detect stuff was removed a while ago and isn't coming back.
That's exactly what I mean - the code was kept simple.
Quoted 6 lines
It is perfectly reasonable for fpga manager drivers to pre-validate the proposed bitstream, IMHO all of the drivers should try and do that step.
Remember, it is deeply unlikely but sending garbage to an FPGA could damage it.
Then what's the purpose of pre-validation? Sending valid data should be the normal case, not the exception.
Quoted 11 lines
Quoted 3 lines
Besides political arguments, there's a more pressing technical argument against this theck as well: The whole check is pointless since the hardware itself already verifies the validity of the stream.
The purpose of the check is not to protect the hardware. The check is to help the user provide the correct input because the hardware provides no feedback at all on what is wrong with the input.
And again, the out-of-tree Xilinx driver accepted files that this driver does not, so having a clear and understandable message is going to be very important for users.
Then just create a "validate my bitstream" tool.
I wrote a Python script to do what Xilinx refused to do years ago:
https://github.com/topic-embedded-products/meta-topic/blob/master/recipes-bsp/fpga/fpga-bit-to-bin/fpga-bit-to-bin.py
So apparently it wasn't hard to figure out what to do.
Quoted 5 lines
Quoted 2 lines
doesn't have any effect, the hardware will discard it. There's no reason to waste CPU cycles duplicating this check in software.
In the typical success case we are talking about 5 32 bit compares, which isn't even worth considering.
I'm mostly against the complication of the code. The code is more complex, and that's bad. It's firmware loading code, and it need not be aware of exactly what it is doing. I did not see any checks like this in other firmware loading code I've come across.
What you're creating here will require active maintenance. There are already 7007 and 7012 devices which aren't in your list so the driver will refuse to load them until someone fills in the IDs. The bitstream format is actually proprietary and undocumented. Any "checks" in code are likely based on guesswork and reverse engineering. We also use partial reprogramming a lot. Did you test that? On all devices? And we're actually planning to go beyond that. Maybe I'll be providing parts of the data through ICAP and some through PCAP, that might even work, but the driver would block it for no apparent reason.
Signature · 16 lines
Mike Looijmans Kind regards, Mike Looijmans System Expert TOPIC Products Materiaalweg 4, NL-5681 RJ Best Postbus 440, NL-5680 AK Best Telefoon: +31 (0) 499 33 69 79 E-mail: mike.looijmans at topicproducts.com Website: www.topicproducts.com Please consider the environment before printing this e-mail
PATCH 1/1 fpga zynq: Check the bitstream for validity
In reply to Mike LooijmansI seem to have missed being CC on this follow up from Mike and wanted to respond:
Quoted 3 lines
What you're creating here will require active maintenance. There are already 7007 and 7012 devices which aren't in your list so the driver will refuse to load them until someone fills in the IDs.
I don't know what list you are refering to, are you sure you are responding to the right patch?
The patch searches for the sync word, it has nothing to do with IDs, and does not attempt to parse any of the proprietary headers. Based on the Xilinx documentation this will work on 7 Series, US and US+ at a minimum. Certainly on all Zynq hardware.
Quoted 3 lines
The bitstream format is actually proprietary and undocumented. Any "checks" in code are likely based on guesswork and reverse engineering.
No, this part is fully documented in UG470.
Quoted 2 lines
We also use partial reprogramming a lot. Did you test that? On all devices?
You should read the patch, it only does the check on a full bitstream.
Jason
PATCH 1/1 fpga zynq: Check the bitstream for validity
In reply to Moritz FischerQuoted 2 lines · Moritz Fischer
That being said, I don't like the idea of the driver having to search either...
I think we are stuck with that, considering what Xilinx tools produce..
Here is a v2, what do you think?
Quoted 1 line
From
93ffde371ca50809ba9b4cdca17051a050b0f92dMon Sep 17 00:00:00 2001
From: Jason Gunthorpe <[email protected]>
Date: Wed, 26 Oct 2016 16:51:26 -0600
Subject: [PATCH v2] fpga zynq: Check the bitstream for validity
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.
--- drivers/fpga/zynq-fpga.c | 35 ++++++++++++++++++++++++++--------- 1 file changed, 26 insertions(+), 9 deletions(-)
drivers/fpga/zynq-fpga.c +26 -9
@@ -175,6 +175,19 @@ static irqreturn_t zynq_fpga_isr(int irq, void *data)return IRQ_HANDLED ; } +/* Sanity check the proposed bitstream. It must start with the sync word in +* the correct byte order. The input is a Xilinx .bin file with every 32 bit +* quantity swapped. +*/ +static bool zynq_fpga_has_sync (const char *buf,size_t count) +{ +for (; count >4 ; buf +=4 , --count) +if (buf[0 ] ==0x66 && buf[1 ] ==0x55 && buf[2 ] ==0x99 && + buf[3 ] ==0xaa ) +return true; +return false; +} +static int zynq_fpga_ops_write_init (struct fpga_manager *mgr,u32 flags,const char *buf,size_t count) {
@@ -184,12 +197,23 @@ static int zynq_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags, priv = mgr->priv; +/* The hardware can only DMA multiples of 4 bytes, and we need at +* least the sync word and something else to do anything. +*/ +if (count <=4 || (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 )) { +if (!zynq_fpga_has_sync (buf, count)) { + err = -EINVAL ; + goto out_err; + } +/* assert AXI interface resets */ regmap_write (priv->slcr,SLCR_FPGA_RST_CTRL_OFFSET ,FPGA_RST_ALL_MASK );
@@ -287,12 +311,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 +339,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 +355,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; }
Signature · 1 line
2.1.4
PATCH 1/1 fpga zynq: Check the bitstream for validity
Quoted 63 lines · Jason Gunthorpe
Quoted 2 lines · Moritz Fischer
That being said, I don't like the idea of the driver having to search either...
I think we are stuck with that, considering what Xilinx tools produce..
Here is a v2, what do you think?
From
93ffde371ca50809ba9b4cdca17051a050b0f92dMon Sep 17 00:00:00 2001
From: Jason Gunthorpe <[email protected]>
Date: Wed, 26 Oct 2016 16:51:26 -0600
Subject: [PATCH v2] fpga zynq: Check the bitstream for validityThere 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 <[email protected]>--- drivers/fpga/zynq-fpga.c | 35 ++++++++++++++++++++++++++--------- 1 file changed, 26 insertions(+), 9 deletions(-)drivers/fpga/zynq-fpga.c +16
-0@@ -175,6 +175,19 @@ static irqreturn_t zynq_fpga_isr(int irq, void *data)return IRQ_HANDLED ; } +/* Sanity check the proposed bitstream. It must start with the sync word in +* the correct byte order. The input is a Xilinx .bin file with every 32 bit +* quantity swapped. +*/ +static bool zynq_fpga_has_sync (const char *buf,size_t count) +{ +for (; count >4 ; buf +=4 , --count) +if (buf[0 ] ==0x66 && buf[1 ] ==0x55 && buf[2 ] ==0x99 && + buf[3 ] ==0xaa ) +return true; +return false; +} +static int zynq_fpga_ops_write_init (struct fpga_manager *mgr,u32 flags,const char *buf,size_t count) {@@ -184,12 +197,23 @@ static int zynq_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags, priv = mgr->priv; +/* The hardware can only DMA multiples of 4 bytes, and we need at +* least the sync word and something else to do anything. +*/
drivers/fpga/zynq-fpga.c static int zynq_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags, · line 212
Quoted 10 lines
drivers/fpga/zynq-fpga.c +4
-0+if (count <=4 || (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 )) { +if (!zynq_fpga_has_sync (buf, count)) {
Maybe we should add an error message here to let the user know what went wrong, as I think this error path could easily be hit by an user.
Regards,
Matthias
Quoted 43 lines
drivers/fpga/zynq-fpga.c +6
-9+ err = -EINVAL ; + goto out_err; + } +/* assert AXI interface resets */ regmap_write (priv->slcr,SLCR_FPGA_RST_CTRL_OFFSET ,FPGA_RST_ALL_MASK );@@ -287,12 +311,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 +339,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 +355,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; }
PATCH 1/1 fpga zynq: Check the bitstream for validity
Quoted 6 lines · Matthias Brugger
Review
Quoted 3 lines
/* don't globally reset PL if we're doing partial reconfig */ if (!(flags & FPGA_MGR_PARTIAL_RECONFIG)) {(quoted diff) +1
-0+ if (!zynq_fpga_has_sync(buf, count)) {Maybe we should add an error message here to let the user know what went wrong, as I think this error path could easily be hit by an user.
Sure, happy with this wording:
drivers/fpga/zynq-fpga.c +7 -2
@@ -181,7 +181,7 @@ static irqreturn_t zynq_fpga_isr(int irq, void *data)*/ static bool zynq_fpga_has_sync (const char *buf,size_t count) { -for (; count >4 ; buf +=4 , --count) +for (; count >4 ; buf +=4 , count -=4 )if (buf[0 ] ==0x66 && buf[1 ] ==0x55 && buf[2 ] ==0x99 && buf[3 ] ==0xaa )return true;
@@ -200,8 +200,11 @@ static int zynq_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags,/* The hardware can only DMA multiples of 4 bytes, and we need at * least the sync word and something else to do anything. */ -if (count <=4 || (count %4 ) !=0 ) +if (count <=4 || (count %4 ) !=0 ) { +dev_err (priv->dev, +"Invalid bitstream size, must be multiples of 4 bytes" );return -EINVAL ; + } err =clk_enable (priv->clk);if (err)
@@ -210,6 +213,8 @@ static int zynq_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags,/* don't globally reset PL if we're doing partial reconfig */ if (!(flags &FPGA_MGR_PARTIAL_RECONFIG )) {if (!zynq_fpga_has_sync (buf, count)) { +dev_err (priv->dev, +"Invalid bitstream, could not find a sync word. Bitstream must be a byte swaped .bin file" ); err = -EINVAL ; goto out_err; }
PATCH 1/1 fpga zynq: Check the bitstream for validity
In reply to Jason GunthorpeQuoted 6 lines · Jason Gunthorpe
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.
Quoted 6 lines
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
PATCH 1/1 fpga zynq: Check the bitstream for validity
Quoted 5 lines · Matthias Brugger
Quoted 2 lines
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_lengthcalculation inzynq_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
PATCH 1/1 fpga zynq: Check the bitstream for validity
Quoted 10 lines · Jason Gunthorpe
Quoted 5 lines · Matthias Brugger
Quoted 2 lines
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_lengthcalculation inzynq_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