RE: [RFC] Add a parser in fpga_region to decode the tlv meta data suggested by Sundar

RE: [RFC] Add a parser in fpga_region to decode the tlv meta data suggested by Sundar

[Nadathur, Sundar]

[Adding linux-kernel mailing list]

> -----Original Message-----
> From: yi1.li@linux.intel.com [mailto:yi1.li@linux.intel.com]
> Sent: Friday, February 24, 2017 9:29 AM
> To: atull@kernel.org; moritz.fischer@ettus.com;
> jgunthorpe@obsidianresearch.com
> Cc: linux-fpga@vger.kernel.org; Nadathur, Sundar
> <sundar.nadathur@intel.com>; Yi Li <yi1.li@linux.intel.com>
> Subject: [RFC] Add a parser in fpga_region to decode the tlv meta data
> suggested by Sundar
> 
> From: Yi Li <yi1.li@linux.intel.com>
> 
> The TLV contains 3 fields
> Type  --- 4 bytes long,  pre-defined
> Length  --- 4 bytes long, Length refers to the length in bytes of the Value field
> Value --- variable length, TLV payload
> 
> In the patch below, there are 5 types, which can be extended.
> TYPE_MDHEADER --- Meta Data Header TLV, which can contain sub TLV types
> TYPE_ENABLETIMEOUT --- Variable type TLV, with 4 bytes long of Value
> TYPE_DISABLETIMEOUT --- Variable type TLV, with 4 bytes long of Value
> TYPE_PARTIAL_RECONFIG --- Flag type TLV, with Zero Length.
> TYPE_BITSTREAM --- Raw bit stream type TLV.
> 
> Example:
> 0x00000001 0x00000014 --- Mata Data header, includes 3 sub-TLVs below
> 0x00000002 0x00000004 0x00000004 --- fpga_image_info.enable_timeout_us
> = 0x4
> 0x00000003 0x00000004 0x00000004 ---
> fpga_image_info.disable_timeout_us = 0x4
> 0x00000005 0x00000000 --- Partial Reconfig Flag
> 0x00000004 0x00100000 --- Bitstream with 1MB long at the end
> 
> Signed-off-by: Yi Li <yi1.li@linux.intel.com>
> ---
>  drivers/fpga/fpga-mgr.c    | 10 ++++-
>  drivers/fpga/fpga-region.c | 93
> +++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 100 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c index
> 3206a53..8d762d328 100644
> --- a/drivers/fpga/fpga-mgr.c
> +++ b/drivers/fpga/fpga-mgr.c
> @@ -311,12 +311,18 @@ EXPORT_SYMBOL_GPL(fpga_mgr_firmware_load);
> 
>  int fpga_mgr_load(struct fpga_manager *mgr, struct fpga_image_info *info)
> {
> +	int ret;
> +
>  	if (info->firmware_name)
>  		return fpga_mgr_firmware_load(mgr, info, info-
> >firmware_name);
>  	if (info->sgt)
>  		return fpga_mgr_buf_load_sg(mgr, info, info->sgt);
> -	if (info->buf && info->count)
> -		return fpga_mgr_buf_load(mgr, info, info->buf, info->count);
> +	if (info->buf && info->count) {
> +		ret = fpga_mgr_buf_load(mgr, info, info->buf, info->count);
> +		info->buf = NULL;
> +		info->count = 0;
> +		return ret;
> +	}
>  	return -EINVAL;
>  }
>  EXPORT_SYMBOL_GPL(fpga_mgr_load);
> diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c index
> a63bc6c..6d8bf47 100644
> --- a/drivers/fpga/fpga-region.c
> +++ b/drivers/fpga/fpga-region.c
> @@ -15,7 +15,7 @@
>   * You should have received a copy of the GNU General Public License along
> with
>   * this program.  If not, see <http://www.gnu.org/licenses/>.
>   */
> -
> +#include <linux/firmware.h>
>  #include <linux/fpga/fpga-bridge.h>
>  #include <linux/fpga/fpga-mgr.h>
>  #include <linux/idr.h>
> @@ -389,6 +389,83 @@ static void fpga_region_put(struct fpga_region
> *region)
>  	mutex_unlock(&region->mutex);
>  }
> 
> +#define TYPE_MDHEADER 0x00000001
> +#define TYPE_ENABLETIMEOUT 0x00000002
> +#define TYPE_DISABLETIMEOUT 0x00000003
> +#define TYPE_BITSTREAM 0x00000004
> +#define TYPE_PARTIAL_RECONFIG 0x00000005
> +
> +/**
> + * fpga_region_parse_header - parser FPGA file header
> + * @data: file header start
> + * @size: file size
> + * @fpga_image_info
> + * @buf: raw bitstream start
> + * @count: raw bitstream size
> + *
> + * Parse the FPGA stream header.
> + *
> + * Return: 0 for success or negative error code.
> + */
> +static int fpga_region_parser_header(const char* data, size_t size,
> +				    struct fpga_image_info *image_info) {
> +	int ret = 0;
> +	u32 tlv_type;
> +	u32 tlv_len;
> +	u32 offset = 0;
> +
> +	while (size >= 8) {
> +		tlv_type = *((u32 *) (data + offset));
> +		tlv_len = *((u32 *) (data + offset + 4));
> +		offset += 8; size -= 8;
> +
> +		if (size < tlv_len)
> +			goto tlv_err;
> +
> +		switch (tlv_type) {
> +			case TYPE_MDHEADER:
> +				break;
> +			case TYPE_ENABLETIMEOUT:
> +				if (tlv_len != sizeof(u32))
> +					goto tlv_err;
> +				image_info->enable_timeout_us = *((u32 *)
> (data + offset));
> +				break;
> +			case TYPE_DISABLETIMEOUT:
> +				if (tlv_len != sizeof(u32))
> +					goto tlv_err;
> +				image_info->disable_timeout_us = *((u32 *)
> (data + offset));
> +				break;
> +			case TYPE_PARTIAL_RECONFIG:
> +				if (tlv_len != 0)
> +					goto tlv_err;
> +				image_info->flags |=
> FPGA_MGR_PARTIAL_RECONFIG;
> +				break;
> +			case TYPE_BITSTREAM:
> +				image_info->buf = data + offset;
> +				image_info->count = tlv_len;
> +				break;
> +			default:
> +				pr_err("unknown type\n");
> +				ret = -EINVAL;
> +		}
> +
> +		/* TYPE_STRUCT is a super type, decode types inside of this
> struct */
> +		if (tlv_type != TYPE_MDHEADER) {
> +			offset += tlv_len;
> +			size -= tlv_len;
> +		}
> +	}
> +
> +	image_info->firmware_name = NULL;
> +	image_info->sgt = NULL;
> +	return ret;
> +
> +tlv_err:
> +	pr_err("Error parsing tlv at type = %d offset = 0x%x\n", tlv_type,
> offset);
> +	return -EINVAL;
> +}
> +
>  /**
>   * fpga_region_program_fpga - program FPGA
>   * @region: FPGA region that is receiving an overlay @@ -401,6 +478,8 @@
> static void fpga_region_put(struct fpga_region *region)  int
> fpga_region_program_fpga(struct fpga_region *region,
>  			     struct fpga_image_info *image_info)  {
> +	struct device *dev = &region->dev;
> +	const struct firmware *fw;
>  	int ret;
> 
>  	region = fpga_region_get(region);
> @@ -415,6 +494,17 @@ int fpga_region_program_fpga(struct fpga_region
> *region,
>  		goto err_put_region;
>  	}
> 
> +	ret = request_firmware(&fw, image_info->firmware_name, dev);
> +	if (ret) {
> +		dev_err(dev, "Error requesting firmware %s\n", image_info-
> >firmware_name);
> +		goto err_put_region;
> +	}
> +
> +	/* todo: parse the header and leave results in image_info */
> +	ret = fpga_region_parser_header(fw->data, fw->size, image_info);
> +	if (ret)
> +		goto err_put_region;
> +
>  	/*
>  	 * In some cases, we already have a list of bridges in the
>  	 * fpga region struct.  Or we don't have any bridges.
> @@ -434,6 +524,7 @@ int fpga_region_program_fpga(struct fpga_region
> *region,
>  	}
> 
>  	ret = fpga_mgr_load(region->mgr, image_info);
> +	release_firmware(fw);
>  	if (ret) {
>  		pr_err("failed to load fpga image\n");
>  		goto err_put_br;
> --
> 2.1.4

RE: [RFC] Add a parser in fpga_region to decode the tlv meta data suggested by Sundar

[Nadathur, Sundar]

On Friday, February 24, 2017 1:17 PM, Jason Gunthorpe wrote:
> Do we need binary data in the header?
> 
> Jason

I have talked about the need for structs and arrays, potentially nested, without really explaining why we may need them eventually. I'll get to that in a moment. But, can we agree that, if nested structs, arrays and general extensibility is needed, FDT or TLVs would be the way to go? If we agree, it becomes a FDT vs. TLV discussion.

Here's why I think we may need extensibility. (Pardon me for starting from the basics -- just trying to set the background.) A FPGA can be programmed as a whole, or can have Partial Reconfiguration (PR) regions, which can be programmed independently. The image programmed into a PR region may have components and sub-units in general. These components may have their own properties. For example, if the FPGA is exposed as a PCIe device to the host, the components may have their own registers in MMIO, DMA attributes and/or interrupts. They may also have identifiers describing their function. In general, we may want these attributes and properties, on  a per-component basis, in the metadata. Even if we don't need them tomorrow, as data center and IOT needs grow, we are likely to need that going forward. 

We could then model the properties of a component as a struct, and the set of components then becomes either an array of structs or a struct of structs. I am not trying to pick a specific object model here-- just mentioning the possibilities. 

IMHO, KVPs are good for scalar quantities. But, when we get to nested arrays/structs, we would need a tree-structured data model, such as TLVs or FDTs. 

Please let me know what you think.

Regards,
Sundar

Re: [RFC] Add a parser in fpga_region to decode the tlv meta data suggested by Sundar

[Jason Gunthorpe]

On Fri, Feb 24, 2017 at 09:41:12PM +0000, Nadathur, Sundar wrote:

> I have talked about the need for structs and arrays, potentially
> nested, without really explaining why we may need them
> eventually. I'll get to that in a moment. But, can we agree that, if
> nested structs, arrays and general extensibility is needed, FDT or
> TLVs would be the way to go? If we agree, it becomes a FDT vs. TLV
> discussion.
> 
> Here's why I think we may need extensibility. (Pardon me for
> starting from the basics -- just trying to set the background.) A
> FPGA can be programmed as a whole, or can have Partial
> Reconfiguration (PR) regions, which can be programmed
> independently. The image programmed into a PR region may have
> components and sub-units in general. These components may have their
> own properties. For example, if the FPGA is exposed as a PCIe device
> to the host, the components may have their own registers in MMIO,
> DMA attributes and/or interrupts. They may also have identifiers
> describing their function. In general, we may want these attributes
> and properties, on a per-component basis, in the metadata. Even if
> we don't need them tomorrow, as data center and IOT needs grow, we
> are likely to need that going forward.

So, you are imagining that the FPGA header would include more than
just information relative to programming it, but also something to do
with operating it?

If that is sensible, then I would go with fdt - it is proven to be
able to handle such complex and open-ended needs and has supporting
tooling.

But.. it is crossing over into the DT overlay stuff, if a complex FPGA
like that is a DT overlay + bitfile then we do not need such data in
the bitfile header.

Jason