[PATCH] fpga zynq: Check the bitstream for validity

[Matthias Brugger]

On 11/08/2016 01:46 AM, Jason Gunthorpe wrote:
> On Fri, Oct 28, 2016 at 05:09:26PM -0700, Moritz Fischer wrote:
>
>> 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 93ffde371ca50809ba9b4cdca17051a050b0f92d Mon Sep 17 00:00:00 2001
> From: Jason Gunthorpe <jgunthorpe at obsidianresearch.com>
> 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.
>
> Signed-off-by: Jason Gunthorpe <jgunthorpe at obsidianresearch.com>
> ---
>  drivers/fpga/zynq-fpga.c | 35 ++++++++++++++++++++++++++---------
>  1 file changed, 26 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/fpga/zynq-fpga.c b/drivers/fpga/zynq-fpga.c
> index c2fb4120bd62..de475a6a1882 100644
> --- a/drivers/fpga/zynq-fpga.c
> +++ b/drivers/fpga/zynq-fpga.c
> @@ -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)) {

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


> +			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;
>  }
>